Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PHOENIX-7381 : Skip last_ddl_timestamp validation when UCF is defined on table and cache entry is not old #1952

Merged
merged 5 commits into from
Oct 1, 2024

Conversation

palashc
Copy link
Contributor

@palashc palashc commented Aug 13, 2024

No description provided.

Comment on lines +1850 to +1853
assertEquals("There should have been no timestamp validation requests.",
0,
newMetricValues.getValidateDDLTimestampRequestsCount()
- oldMetricValues.getValidateDDLTimestampRequestsCount());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

newMetricValues.getValidateDDLTimestampRequestsCount() should also be 0?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There will be requests from other tests, that's why I am checking the difference before and after just this test.

@palashc
Copy link
Contributor Author

palashc commented Sep 12, 2024

@virajjasani Does this change look good? If yes, we can get it in.

Copy link
Contributor

@virajjasani virajjasani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few nits, +1 otherwise

@@ -40,7 +40,7 @@ private static void initCluster() throws Exception {
Map<String, String> props = Maps.newConcurrentMap();
props.put(BaseScannerRegionObserverConstants.PHOENIX_MAX_LOOKBACK_AGE_CONF_KEY,
Integer.toString(60 * 60 * 1000));
props.put(QueryServices.DEFAULT_UPDATE_CACHE_FREQUENCY_ATRRIB, "ALWAYS");
props.put(QueryServices.DEFAULT_UPDATE_CACHE_FREQUENCY_ATRRIB, "NEVER");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we want to test with both ALWAYS and NEVER?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@virajjasani With DDL timestamp validation, UCF should not be ALWAYS, so I think it is okay. WDYT?

Copy link
Contributor

@virajjasani virajjasani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, pending jenkins build results

@palashc palashc merged commit 3ac1774 into apache:master Oct 1, 2024
1 check failed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants