Uh oh!
There was an error while loading. Please reload this page.
PHOENIX-5435 - Annotate HBase WALs with Phoenix Metadata - #913
Merged
Conversation
stoty
commented
Oct 7, 2020
Contributor
💔 -1 overall
This message was automatically generated. |
stoty
commented
Oct 12, 2020
Contributor
💔 -1 overall
This message was automatically generated. |
stoty
commented
Oct 13, 2020
Contributor
💔 -1 overall
This message was automatically generated. |
ChinmaySKulkarni
left a comment
Contributor
There was a problem hiding this comment.
Did a quick glance and this seems like a really cool feature! Overall lgtm and nice tests.
Will try to do a more in-depth review later.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| <!-- See BUILDING.md for profile selection --> | ||
| <profile> | ||
| <id>phoenix-hbase-compat-1.3.0-default</id> | ||
| <id>phoenix-hbase-compat-1.5.0-default</id> |
Contributor
There was a problem hiding this comment.
Why did we change this?
ContributorAuthor
There was a problem hiding this comment.
With 1.3 as the default, it's difficult to run the 1.5-specific tests like WALAnnotationIT in IntelliJ, and it means they won't get exercised in the automated test runs. FYI, @stoty .
Contributor
There was a problem hiding this comment.
Since 4.x multibranch keep running all tests with all supported versions including 1.5, pom.xml changes will be reverted before merging this PR?
ChinmaySKulkarni
commented
Nov 11, 2020
Contributor
@gjacoby126 Some high-level questions:
|
gjacoby126
commented
Nov 11, 2020
ContributorAuthor
A critique I received offline to this PR from @priyankporwal suggested that annotating indexes might not be worth the storage cost, so we could also consider exempting indexes in the next draft (which is waiting on PHOENIX-6186 to populate the DDL timestamps) |
gjacoby126force-pushed
the
PHOENIX-5435
branch
from
November 20, 2020 22:35
976ae42 to
9c1f0d1Comparestoty
commented
Nov 21, 2020
Contributor
💔 -1 overall
This message was automatically generated. |
stoty
commented
Nov 26, 2020
Contributor
💔 -1 overall
This message was automatically generated. |
stoty
commented
Nov 30, 2020
Contributor
💔 -1 overall
This message was automatically generated. |
gjacoby126
commented
Dec 1, 2020
ContributorAuthor
In this latest revision:
I believe the one remaining request left to do is @kadirozde 's suggestion to do a perf test. I will do this over the next couple of days. @ChinmaySKulkarni , @kadirozde , fyi. |
stoty
commented
Dec 2, 2020
Contributor
💔 -1 overall
This message was automatically generated. |
gjacoby126
commented
Dec 2, 2020
ContributorAuthor
The test run is odd -- looks like phoenix-core passed but then there was an error in building or shading phoenix-client? I can't reproduce locally -- mvn clean install -DskipTests passes for me. |
stoty
commented
Dec 4, 2020
Contributor
💔 -1 overall
This message was automatically generated. |
stoty
commented
Dec 8, 2020
Contributor
💔 -1 overall
This message was automatically generated. |
virajjasani
left a comment
Contributor
There was a problem hiding this comment.
Left few comments while trying to get familiarity with this nice feature
| private void annotateDataMutations(UngroupedAggregateRegionObserver.MutationList mutationsList, | ||
| Scan scan) { | ||
| byte[] tenantId = null; |
Contributor
There was a problem hiding this comment.
nit: we are assigning these variables representing MutationMetadataType twice (null and scan.getAttribute()), should we assign all of them just once by removing null assignments?
Uh oh!
There was an error while loading. Please reload this page.
| lastDDLTimestampKv.getValueOffset(), SortOrder.getDefault()); | ||
| Cell changeDetectionEnabledKv = tableKeyValues[CHANGE_DETECTION_ENABLED_INDEX]; | ||
| boolean isChangeDetectionEnabled = changeDetectionEnabledKv != null |
Contributor
There was a problem hiding this comment.
In case of old client - new server case, changeDetectionEnabledKv will be null right?
ContributorAuthor
There was a problem hiding this comment.
So long as some other client hasn't set CHANGE_DETECTION_ENABLED=true on the table via an ALTER statement.
Contributor
There was a problem hiding this comment.
If a new client modifies some table's CDE field, there won't be a problem for old clients interacting with the table right?
ContributorAuthor
There was a problem hiding this comment.
Since it's an optional field in the protobuf and the old client won't even know to deserialize it I don't think it would cause a problem.
| } | ||
| public static void annotateMutationWithMetadata(PTable table, Mutation mutation) { |
Contributor
There was a problem hiding this comment.
nit: we can keep both util methods private?
Comment on lines
+31
to
+32
| public static String PHOENIX_APPEND_METADATA_TO_WAL = "phoenix.append.metadata.to.wal"; | ||
| public static boolean DEFAULT_PHOENIX_APPEND_METADATA_TO_WAL = false; |
Contributor
There was a problem hiding this comment.
nit: should we avoid defining constants in Compat classes?
ContributorAuthor
There was a problem hiding this comment.
These aren't used, will remove.
| MutationState.MutationMetadataType.values()) { | ||
| String metadataTypeKey = metadataType.toString(); | ||
| if (attrMap.containsKey(metadataTypeKey)) { | ||
| CompatIndexRegionObserver.appendToWALKey(key, metadataTypeKey, |
Contributor
There was a problem hiding this comment.
appendToWALKey() is public only in 1.5 compact class but not in 1.3/1.4 compact classes, without which this could cause compilation issue while running against 1.3/1.4.
Contributor
There was a problem hiding this comment.
Moreover, 1.3/1.4 compact classes don't have it as static
ContributorAuthor
There was a problem hiding this comment.
Oops, thanks for the catch.
| if (isChangeDetectionEnabledProp != null) { | ||
| if (tableType != TABLE && tableType != VIEW) { | ||
| throw new SQLExceptionInfo.Builder( | ||
| SQLExceptionCode.CHANGE_DETECTION_SUPPORTED_FOR_TABLES_AND_VIEWS_ONLY) |
Contributor
There was a problem hiding this comment.
If possible, we can refactor this validation in a small util method and it can be used by both workflows: createTable/createIndex and addColumn/alterIndex?
gokceni
reviewed
Dec 9, 2020
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| * Used for an aggregate query in which the key order does not necessarily match the group by | ||
| * key order. In this case, we must collect all distinct groups within a region into a map, | ||
| * aggregating as we go. | ||
| * @param limit TODO |
Contributor
There was a problem hiding this comment.
TODO seems to be dropped but not implemented?
ContributorAuthor
There was a problem hiding this comment.
Removed the params from the Javadoc as they were Javadoc errors.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| /** | ||
| * <code>optional string logicalIndexName = 25;</code> | ||
| */ | ||
| java.lang.String getLogicalIndexName(); |
Contributor
There was a problem hiding this comment.
Would this be populated for both view indexes and regular indexes and local indexes?
Contributor
There was a problem hiding this comment.
@gjacoby126 thanks for addressing my comments. There is just this question remaining.
ContributorAuthor
There was a problem hiding this comment.
@gokceni Correct, should be populated for all indexes.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
ChinmaySKulkarni
left a comment
Contributor
There was a problem hiding this comment.
Really cool feature! Thanks @gjacoby126. I've added some comments and questions.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
stoty
commented
Dec 10, 2020
Contributor
💔 -1 overall
This message was automatically generated. |
gjacoby126
commented
Dec 11, 2020
Uh oh!
There was an error while loading. Please reload this page.
gjacoby126force-pushed
the
PHOENIX-5435
branch
from
December 11, 2020 23:06
3d85019 to
8c396eaComparestoty
commented
Dec 12, 2020
Contributor
💔 -1 overall
This message was automatically generated. |
gjacoby126force-pushed
the
PHOENIX-5435
branch
from
December 14, 2020 17:16
8c396ea to
3c25fc8Comparegjacoby126
commented
Dec 14, 2020
ContributorAuthor
@gokceni , @ChinmaySKulkarni , @virajjasani , I believe I have addressed all outstanding comments (and also pushed up a fix for an NPE in ViewIndexIT the last test run found.) Could you please give it another look? Thanks! |
Contributor
@gjacoby126 btw for the past couple of days, all precommit PR builds for 4.x (that I came across) are timing out and precommit Jira builds for 4.x patch are passing by ~1/4 probability (that I am aware of) so you might also want to upload 4.x patch to Jira to trigger 4.x precommit Jira build. |
ChinmaySKulkarni
left a comment
Contributor
There was a problem hiding this comment.
+1 thanks @gjacoby126
ChinmaySKulkarni
approved these changes
Dec 14, 2020
stoty
commented
Dec 14, 2020
Contributor
💔 -1 overall
This message was automatically generated. |
virajjasani
approved these changes
Dec 15, 2020
| <!-- See BUILDING.md for profile selection --> | ||
| <profile> | ||
| <id>phoenix-hbase-compat-1.3.0-default</id> | ||
| <id>phoenix-hbase-compat-1.5.0-default</id> |
Contributor
There was a problem hiding this comment.
Since 4.x multibranch keep running all tests with all supported versions including 1.5, pom.xml changes will be reverted before merging this PR?
gokceni
commented
Dec 15, 2020
Contributor
@gjacoby126 +1 Thanks! |
gokceni
approved these changes
Dec 15, 2020
gjacoby126
commented
Dec 15, 2020
ContributorAuthor
Thanks for the reviews, @ChinmaySKulkarni@gokceni and @virajjasani ! |
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
5 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.