-
Notifications
You must be signed in to change notification settings - Fork 1k
PHOENIX-4983: Allow using a connection with a SCN set to write data to tables EXCEPT transactional tables or mutable tables with indexes or tables with ROW_TIMESTAMP column. #405
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
Conversation
7e94cd4 to
fa96a5c
Compare
twdsilva
left a comment
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.
@swaroopak thanks for the PR, I have left some feedback.
|
|
||
| } | ||
| @Test | ||
| public void testUpsertOnSCNSetMutTable() throws Exception { |
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.
add a helper function to test the tables that are mutable/immutable and which don't have an index, have a global index or a local index (and pass these options in as parameters).
| @Test // See https://issues.apache.org/jira/browse/PHOENIX-4983 | ||
| public void testUpsertOnSCNSetTxnTable() throws SQLException { | ||
|
|
||
| String trnx = "CREATE TABLE transaction_table (METRIC_ID CHAR(15) NOT NULL,\n" + |
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.
Use a generateUniqueName() to create tables with unique names since tests that extend ParallelStatsDisabledIT
share the same cluster.
| CANNOT_UPSERT_WITH_SCN_FOR_ROW_TIMSTAMP_COLUMN(901,"43M12", | ||
| "Cannot use a connection with SCN set to upsert data for " + | ||
| "table with ROW_TIMESTAMP column."), | ||
| CANNOT_UPSERT_WITH_SCN_FOR_TXN_TABLE(902,"43M13", |
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.
remove this as its not used
| if (plan.getTargetRef() != null && plan.getTargetRef().getTable() != null && plan.getTargetRef().getTable().isTransactional()) { | ||
| state.startTransaction(plan.getTargetRef().getTable().getTransactionProvider()); | ||
| PTable table = plan.getTargetRef().getTable(); | ||
| if(table != null && connection.getSCN() != null && !connection.isRunningUpgrade() && !connection.isBuildingIndex()) { |
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.
can you move these checks to UpsertCompiler? Also I think you can remove the second check for mutable tables with indexes here.
c6ce8be to
c19b2a7
Compare
|
|
||
| helpTestUpserWithSCNIT(false, true, false, false, false); | ||
| exception.expect(SQLException.class); | ||
| exception.expectMessage(containsString("ERROR 1075 (44A06): Cannot use a " + |
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.
don't compare error message strings, use errorCode instead.
twdsilva
left a comment
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.
@swaroopak looks good, please make the change that karan requested and we can get this committed.
| } | ||
|
|
||
| @Test | ||
| public void testUpsertOnSCNSetMutTable() throws Exception { |
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.
nit: rename to testUpsertOnSCNSetMutableTableWithoutIndexes
cda3ff6 to
6a6c858
Compare
|
+1 |
…o tables EXCEPT transactional tables or mutable tables with indexes or tables with ROW_TIMESTAMP column.
6a6c858 to
03b746c
Compare
…