Uh oh!
There was an error while loading. Please reload this page.
PHOENIX-6186 - Store last DDL timestamp in System.Catalog - #935
Merged
Conversation
stoty
commented
Oct 23, 2020
Contributor
💔 -1 overall
This message was automatically generated. |
gjacoby126
commented
Oct 26, 2020
ContributorAuthor
Three tests failed (IndexExtendedIT, UpgradeIT, ImmutableIndexIT), but two were timeouts (the index tests), and all pass when run locally so appear to be flappers. |
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
Oct 28, 2020
Contributor
💔 -1 overall
This message was automatically generated. |
stoty
commented
Oct 29, 2020
Contributor
💔 -1 overall
This message was automatically generated. |
gjacoby126
commented
Oct 30, 2020
ContributorAuthor
@ChinmaySKulkarni - I believe this takes care of all the issues we've discussed. Please let me know if there's any other issues you see.
|
stoty
commented
Oct 31, 2020
Contributor
💔 -1 overall
This message was automatically generated. |
stoty
commented
Nov 5, 2020
Contributor
💔 -1 overall
This message was automatically generated. |
gjacoby126
commented
Nov 5, 2020
ContributorAuthor
@ChinmaySKulkarni - After fixing the test failure in UpgradeIT, we got another test run. It looks like in this latest run the minicluster got into a bad state where tables couldn't be created, and this led to a bunch of failures. I've rerun them all locally and they look fine. As far as I know, this patch should be ready to go for 4.x branch. Anything else that needs to be changed or added? Thanks! |
ChinmaySKulkarni
left a comment
Contributor
There was a problem hiding this comment.
@gjacoby126 thanks for addressing my previous comments. I have a few more comments after the latest changes. Overall, it looks really good.
The column add/drop inheritance to child views might be tricky given all the scenarios (diverged views, older clients, existing logic for combining parent columns given a split SYSTEM.CATALOG, nuances when phoenix.allow.system.catalog.rollback=true, etc.).
Should we leave that out in this one and handle it in a separate Jira?
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
Nov 12, 2020
Contributor
💔 -1 overall
This message was automatically generated. |
shahrs87
requested changes
Nov 12, 2020
shahrs87
left a comment
Contributor
There was a problem hiding this comment.
@gjacoby126 I am not that much active in phoenix project. Tried to review to the best of my knowledge. Thank you !
| long clientVersion) { | ||
| long clientVersion, | ||
| boolean isAddingColumns, | ||
| List<PTable> childViews) { |
Contributor
There was a problem hiding this comment.
Do we need to add childViews to validateAndAddMetadata method ? Maybe I am reading it wrong but I don't see childViews getting used either in AddColumnMutator#validateAndAddMetadata or DropColumnMutator#validateAndAddMetadata.
ContributorAuthor
There was a problem hiding this comment.
Good catch, it was necessary in a prior draft but the code that used it was just removed. I'll remove the parameter.
| //we don't need to update the DDL timestamp for any child views we may have, because | ||
| // when we look up a PTable for any of those child views, we'll take the max timestamp | ||
| // of the view and all its ancestors | ||
| tableMetaData.addAll(additionalTableMetaData); |
Contributor
There was a problem hiding this comment.
tableMetaData.addAll(additionalTableMetaData);
The above line won't be executed if isDroppingColumns is false. Earlier it was getting executed even if we are dropping columns.
| } | ||
| tableMetaData.addAll(additionalTableMetaData); | ||
| if (isDroppingColumns) { |
Contributor
There was a problem hiding this comment.
The changes here looks exactly same as the changes in AddColumnMutator. Can we create a helper method which accepts table and additionalTableMetaData as argument ?
ContributorAuthor
There was a problem hiding this comment.
It's not really general-purpose enough to put in a standalone Util method in MetaDataUtil, and since it's only 3 lines repeated in 2 places I figured the minor violation of DRY (Don't Repeat Yourself) wasn't worth working around, since all alternatives I could think of seemed a little ugly.
For example, I could put a static utility method in one of the mutators, but then one depends on the other which couples classes that shouldn't be...
Uh oh!
There was an error while loading. Please reload this page.
| */ | ||
| EncodedCQCounter cqCounterToBe = tableType == PTableType.VIEW ? NULL_COUNTER : cqCounter; | ||
| PTable table = new PTableImpl.Builder() | ||
| .setType(tableType) |
Contributor
There was a problem hiding this comment.
Here we are adding just 1 filed to builder method. Can we please undo the formatting changes which is making the diff look more than actual changes.
ContributorAuthor
There was a problem hiding this comment.
Sorry for the inconvenience, but the formatting changes fix the spacing from being incorrect to correct. GitHub does have a "Hide whitespace changes" in the setting icon near the top of the page, btw, to make it easier to ignore.
Contributor
There was a problem hiding this comment.
I know this is a very nit pick.. but for split lines the spacing rule is 8 spaces. For conditional/loop statements body I agree it is 4 spaces.
Uh oh!
There was an error while loading. Please reload this page.
shahrs87
requested changes
Nov 13, 2020
shahrs87
left a comment
Contributor
There was a problem hiding this comment.
Changes looks good to me. almost all are nits and mostly in test related code. Thank you @gjacoby126 !
| dataTableFullName, startTS, | ||
| conn); | ||
| Thread.sleep(1); | ||
| //now add a column and make sure the timestamp updates |
Contributor
There was a problem hiding this comment.
is this comment carried forward from previous test and no longer relevant here ?
| String columnAddDDL = "ALTER TABLE " + dataTableFullName + " ADD COL3 varchar(50) NULL "; | ||
| String columnDropDDL = "ALTER TABLE " + dataTableFullName + " DROP COLUMN COL3 "; | ||
| long startTS = EnvironmentEdgeManager.currentTimeMillis(); | ||
| try (Connection conn = DriverManager.getConnection(getUrl(), props)) { |
Contributor
There was a problem hiding this comment.
just 1 minor nit. DriverManager has method getConnection without Properties argument. We can use that in all newly added test cases since we don't override any properties here. If it is too much change please feel free to ignore also.
| conn.createStatement().execute(divergeDDL); | ||
| //verify DDL timestamp changed | ||
| viewDDLTimestamp = CreateTableIT.verifyLastDDLTimestamp(schemaName, viewName, | ||
| viewFullName, viewDDLTimestamp, conn); |
Contributor
There was a problem hiding this comment.
Acc to comment, shouldn't the argument for startTime be "viewDDLTimestamp + 1" since we are assuming that timestamp should change.
| viewFullName, viewDDLTimestamp, conn); | ||
| conn.createStatement().execute(viewColumnAddDDL); | ||
| //verify DDL timestamp changed because we added a column to the view | ||
| viewDDLTimestamp = CreateTableIT.verifyLastDDLTimestamp(schemaName, viewName, |
Contributor
There was a problem hiding this comment.
I understand it is strictly not required but should we sleep for 1 millisecond just to make sure that 1 ms passed between divergeDDL and viewColumnAddDDL execution and the same argument applies for other ddl statements that we execute subsequently in this test.
Uh oh!
There was an error while loading. Please reload this page.
| } | ||
| public static long verifyLastDDLTimestamp(String tenantId, String schemaName, String tableName, | ||
| String dataTableFullName, long startTS, Connection conn) throws SQLException { |
Contributor
There was a problem hiding this comment.
I don't see any usage of first 3 arguments String tenantId, String schemaName, String tableName
Maybe you used them in earlier draft then removed them ?
| result = mutator.validateAndAddMetadata(table, rowKeyMetaData, tableMetadata, | ||
| region, invalidateList, locks, clientTimeStamp, clientVersion); | ||
| region, invalidateList, locks, clientTimeStamp, clientVersion, | ||
| isAddingOrDroppingColumns); |
Uh oh!
There was an error while loading. Please reload this page.
| */ | ||
| EncodedCQCounter cqCounterToBe = tableType == PTableType.VIEW ? NULL_COUNTER : cqCounter; | ||
| PTable table = new PTableImpl.Builder() | ||
| .setType(tableType) |
Contributor
There was a problem hiding this comment.
I know this is a very nit pick.. but for split lines the spacing rule is 8 spaces. For conditional/loop statements body I agree it is 4 spaces.
Uh oh!
There was an error while loading. Please reload this page.
gjacoby126
commented
Nov 13, 2020
ContributorAuthor
@shahrs87 - I'm curious -- what is the source is for the "split lines indent 8 spaces" rule you mention? You may very well be right, but I don't see anything in dev/PhoenixCodeTemplate.xml that has a value of 8 that looks relevant, and I do see |
shahrs87
commented
Nov 13, 2020
Contributor
Tried searching for the property name but couldn't find one. But other places in phoenix code base uses 8 spaces. Eg: https://github.com/apache/phoenix/blob/master/phoenix-core/src/main/java/org/apache/phoenix/schema/PTableImpl.java#L883, https://github.com/apache/phoenix/blob/master/phoenix-core/src/main/java/org/apache/phoenix/schema/PTableImpl.java#L1042 |
Contributor
Actually found the property. It uses this property: |
stoty
commented
Nov 13, 2020
Contributor
💔 -1 overall
This message was automatically generated. |
gjacoby126
commented
Nov 13, 2020
ContributorAuthor
Reran IndexMetaDataIT and ViewMetadataIT locally and they both passed. |
stoty
commented
Nov 13, 2020
Contributor
💔 -1 overall
This message was automatically generated. |
shahrs87
commented
Nov 13, 2020
Contributor
@gjacoby126 Are any checkstyle, javadocs, spotbugs, findbugs warnings relevant here ? I know that there is some noise but don't know whether any of them are related to patch ? |
gjacoby126
commented
Nov 16, 2020
ContributorAuthor
@shahrs87 - Of the three findbugs errors, the first is unclear to me, the second is complaining about a potential resource leak in the boostrap code (valid complaint) and the third is complaining about a potential SQL injection vulnerability (incorrect, but understandable). I've changed it to use a PreparedStatement in a try-with-resources executing a constant string. Went through the checkstyle findings. The line lengths checks looks misconfigured (it's 80 but should be 100, and we routinely let things a little longer than 100 slide). I did go through and fix a bunch of whitespace complaints around if and for blocks, plus marking some params final, to clear out some of the noise. |
gjacoby126
commented
Nov 16, 2020
ContributorAuthor
@ChinmaySKulkarni , @shahrs87 , assuming the test runs are OK, are there any other changes you'd like, or is this ready to go? Thanks for your reviews. |
stoty
commented
Nov 16, 2020
Contributor
💔 -1 overall
This message was automatically generated. |
gjacoby126
commented
Nov 16, 2020
ContributorAuthor
The two test failures were both timeouts and passed locally. |
stoty
commented
Nov 16, 2020
Contributor
💔 -1 overall
This message was automatically generated. |
shahrs87
approved these changes
Nov 17, 2020
Contributor
There was a problem hiding this comment.
@gjacoby126 Looks like some minor nits in test code is yet to be addressed from my previous review. After that I am +1 (non-binding). Already approving the PR. Thank you !
| String columnAddDDL = "ALTER TABLE " + dataTableFullName + " ADD COL3 varchar(50) NULL "; | ||
| String columnDropDDL = "ALTER TABLE " + dataTableFullName + " DROP COLUMN COL3 "; | ||
| long startTS = EnvironmentEdgeManager.currentTimeMillis(); | ||
| try (Connection conn = DriverManager.getConnection(getUrl(), props)) { |
| dataTableFullName, startTS, | ||
| conn); | ||
| Thread.sleep(1); | ||
| //now add a column and make sure the timestamp updates |
Uh oh!
There was an error while loading. Please reload this page.
| try (Connection tenantConn = DriverManager.getConnection(getUrl(), props)) { | ||
| tenantConn.createStatement().execute(tenantViewDDL); | ||
| } | ||
| // First, check that adding a child view didn't change the timestamps |
stoty
commented
Nov 17, 2020
Contributor
💔 -1 overall
This message was automatically generated. |
gjacoby126force-pushed
the
PHOENIX-6186
branch
from
November 17, 2020 21:58
6855823 to
7f6c989CompareChinmaySKulkarni
requested changes
Nov 17, 2020
ChinmaySKulkarni
left a comment
Contributor
There was a problem hiding this comment.
@gjacoby126 Added some more comments. Most are nits, but the more important ones are related to the handling of diverged views when a column is dropped from their ancestor. I've added more details in other comments.
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.
| // that uses encoded column qualifiers (the remote mutations are to update the | ||
| // encoded column qualifier counter on the parent table) | ||
| if (mutator.getMutateColumnType() == ColumnMutator.MutateColumnType.ADD_COLUMN | ||
| if (childViews.size() > 0 || ( mutator.getMutateColumnType() == ColumnMutator.MutateColumnType.ADD_COLUMN |
Contributor
There was a problem hiding this comment.
Shouldn't this be done even if if (!remoteMutations.isEmpty()) is false if the table/view has child views??
ContributorAuthor
There was a problem hiding this comment.
I added the childViews.size() check when I was sending the ddl timestamp mutations remotely to the child view header rows, which I stopped doing in the last draft. That part of the if clause should just be removed right? Or was the existing logic wrong if a child view existed?
Uh oh!
There was an error while loading. Please reload this page.
stoty
commented
Nov 18, 2020
Contributor
💔 -1 overall
This message was automatically generated. |
stoty
commented
Nov 18, 2020
Contributor
💔 -1 overall
This message was automatically generated. |
gjacoby126force-pushed
the
PHOENIX-6186
branch
from
November 18, 2020 21:53
ac5daf5 to
75c5bdfComparestoty
commented
Nov 19, 2020
Contributor
💔 -1 overall
This message was automatically generated. |
stoty
commented
Nov 19, 2020
Contributor
💔 -1 overall
This message was automatically generated. |
ChinmaySKulkarni
approved these changes
Nov 20, 2020
ChinmaySKulkarni
left a comment
Contributor
There was a problem hiding this comment.
Lgtm @gjacoby126. Thanks for this really useful feature!
gjacoby126
commented
Nov 20, 2020
ContributorAuthor
Thanks for the reviews, @ChinmaySKulkarni and @shahrs87 ! Squashed and merged. I've opened up a PR at #977 for the master branch and will also commit there if tests are green. |
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.