Uh oh!
There was an error while loading. Please reload this page.
PHOENIX-6179 : Relax maxLookBackAge check during an upgrade - #942
Closed
virajjasani wants to merge 1 commit into
Closed
PHOENIX-6179 : Relax maxLookBackAge check during an upgrade#942virajjasani wants to merge 1 commit into
virajjasani wants to merge 1 commit into
Conversation
stoty
commented
Oct 28, 2020
Contributor
💔 -1 overall
This message was automatically generated. |
virajjasani
commented
Oct 28, 2020
ContributorAuthor
@gjacoby126@jpisaac@ChinmaySKulkarni Could you please take a look? 4.x patch is also attached to Jira. |
virajjasani
commented
Oct 29, 2020
ContributorAuthor
FYI @yanxinyi |
| Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES); | ||
| if (setMaxLookBackAge) { | ||
| props.put( | ||
| QueryServices.GLOBAL_INDEX_ROW_AGE_THRESHOLD_TO_DELETE_MS_ATTRIB, |
Contributor
There was a problem hiding this comment.
why do we need to set this property?
ContributorAuthor
There was a problem hiding this comment.
My bad, this is not needed, it came as part of copy from existing test. Removed it.
Thanks
| QueryServices.GLOBAL_INDEX_ROW_AGE_THRESHOLD_TO_DELETE_MS_ATTRIB, | ||
| Long.toString(0)); | ||
| props.put(CompatBaseScannerRegionObserver.PHOENIX_MAX_LOOKBACK_AGE_CONF_KEY, | ||
| Integer.toString(15)); |
ContributorAuthor
There was a problem hiding this comment.
With 15, we are able to reproduce the exception without source code change on this patch. Is it fine? I can update this if there is any better suggestion that I might be unaware of.
ContributorAuthor
There was a problem hiding this comment.
Any value less than 31 should work for testing purpose. Because during upgrade, SCN is set to be Phoenix version timestamp:
public static final long MIN_SYSTEM_TABLE_TIMESTAMP_4_16_0 = MIN_TABLE_TIMESTAMP + 31;
public static final long MIN_SYSTEM_TABLE_TIMESTAMP_5_1_0 = MIN_SYSTEM_TABLE_TIMESTAMP_4_16_0;
Contributor
There was a problem hiding this comment.
can you add explanation comments here so that in the future ppl have a better understanding, thanks
stoty
commented
Oct 30, 2020
Contributor
💔 -1 overall
This message was automatically generated. |
stoty
commented
Oct 30, 2020
Contributor
💔 -1 overall
This message was automatically generated. |
jpisaac
reviewed
Oct 30, 2020
jpisaac
left a comment
Contributor
There was a problem hiding this comment.
LGTM, added a suggestion, Thanks for the patch
| // Executes the SQL commands listed in the given operation file from the sql_files directory | ||
| private void executeQueriesWithCurrentVersion(String operation) throws Exception { | ||
| private void executeQueriesWithCurrentVersion(String operation, | ||
| boolean setMaxLookBackAge) throws Exception { |
Contributor
There was a problem hiding this comment.
@virajjasani Was wondering whether you would entertain the idea of using an EnumSet instead of boolean (which is specific to maxLookBaackAge). The thinking was we could then reuse this method more easily for other property checks and combinations of them.
yanxinyi
approved these changes
Oct 30, 2020
stoty
commented
Oct 30, 2020
Contributor
💔 -1 overall
This message was automatically generated. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.