-
Notifications
You must be signed in to change notification settings - Fork 994
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-6032: When phoenix.allow.system.catalog.rollback=true, a view still sees data from a column that was dropped #949
Conversation
Please review @yanxinyi @jpisaac @gjacoby126 @twdsilva |
phoenix-core/src/it/java/org/apache/phoenix/end2end/SystemCatalogRollbackEnabledIT.java
Outdated
Show resolved
Hide resolved
phoenix-core/src/it/java/org/apache/phoenix/end2end/SystemCatalogRollbackEnabledIT.java
Outdated
Show resolved
Hide resolved
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
45f6a82
to
73e597e
Compare
Fixed whitespace and checkStyle warnings. Also, test failures are not failing locally. |
💔 -1 overall
This message was automatically generated. |
73e597e
to
63812e7
Compare
Thanks for the +1 Xinyi. Any one else want to review this patch? @virajjasani @gjacoby126 ? |
63812e7
to
bd51b43
Compare
Deleted last build to avoid noise. Waiting for latest build |
💔 -1 overall
This message was automatically generated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few minor nits, +1 (non-binding) for source changes with my understanding
phoenix-core/src/it/java/org/apache/phoenix/end2end/SystemCatalogRollbackEnabledIT.java
Show resolved
Hide resolved
phoenix-core/src/it/java/org/apache/phoenix/end2end/SystemCatalogRollbackEnabledIT.java
Outdated
Show resolved
Hide resolved
phoenix-core/src/it/java/org/apache/phoenix/end2end/SystemCatalogRollbackEnabledIT.java
Outdated
Show resolved
Hide resolved
9f69925
to
e5e08f0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just a few nits
testUtil.getHBaseAdmin().enableTable(systemCatalog); | ||
} catch (DoNotRetryIOException e) { | ||
// table is not splittable | ||
assert (e.getMessage().contains("NOT splittable")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we require that we get this exception? If so should be an Assert.fail() after enableTable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, should use JUnit assertions rather than Java assert keyword
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I know this is a copied test, but good to do the simple fixes while we're at it)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm I'm not sure. This diff is just a result of copy pasting from the original test SystemCatalogIT
Let me try to find out. Will change to using JUnit assert.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like that test wasn't reliably trying to split SYSCAT at all. I will open a follow-up Jira to fix this since it is unrelated to 6032.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
phoenix-core/src/it/java/org/apache/phoenix/end2end/SystemCatalogRollbackEnabledIT.java
Show resolved
Hide resolved
e5e08f0
to
a62205a
Compare
… still sees data from a column that was dropped
Thanks for your reviews @virajjasani @gjacoby126 I have addressed all your review comments. Can I get a +1? |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, thanks @ChinmaySKulkarni
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
No description provided.