Uh oh!
There was an error while loading. Please reload this page.
PHOENIX-6083 View index creation does a checkAndPut on an incorrect r… - #929
Merged
Merged
Conversation
richardantalforce-pushed
the
PHOENIX-6083
branch
from
October 19, 2020 16:29
a3ea120 to
0823711Comparerichardantal
commented
Oct 19, 2020
ContributorAuthor
@ChinmaySKulkarni Can you review this change please? For the view index creation we created a mutex for |
stoty
commented
Oct 19, 2020
Contributor
💔 -1 overall
This message was automatically generated. |
ChinmaySKulkarni
left a comment
Contributor
There was a problem hiding this comment.
Overall looks good (have a few small comments). Thanks for fixing this @richardantal!
| String fullViewName = SchemaUtil.getTableName(viewSchemaName, viewName); | ||
| createBaseTable(schemaName, tableName, false, null, null, true); | ||
| Connection conn = getConnection(); |
Contributor
There was a problem hiding this comment.
nit: try-with-resources for the connection
| try { | ||
| conn.createStatement().execute("CREATE INDEX " + indexName + " ON " + fullViewName + " (v1)"); | ||
| } catch (SQLException e) { | ||
| failedMsg[0] = e.getMessage(); |
Contributor
There was a problem hiding this comment.
nit: Can we store and hence later assert the SQLException code rather than the exception message?
| for (ColumnName colName : requiredCols) { | ||
| // acquire the mutex using the global physical table name to | ||
| // prevent this column from being dropped while the index is being created | ||
| String colNameSeparatedByDot = colName.getColumnName().replace(':', '.'); |
Contributor
There was a problem hiding this comment.
Good to either extract this out as a SchemaUtil API and/or add a comment here. Is there some existing API which makes the indexed column name have a ':' instead of '.'?
ChinmaySKulkarni
commented
Oct 19, 2020
Contributor
Looks like |
stoty
commented
Oct 19, 2020
Contributor
💔 -1 overall
This message was automatically generated. |
richardantal
commented
Oct 20, 2020
ContributorAuthor
|
richardantal
commented
Oct 20, 2020
ContributorAuthor
Thank you@ChinmaySKulkarni for the review! I have updated the PR according to the comments. |
stoty
commented
Oct 20, 2020
Contributor
💔 -1 overall
This message was automatically generated. |
richardantalforce-pushed
the
PHOENIX-6083
branch
from
October 30, 2020 15:25
74d254c to
64d4b74Comparerichardantal
commented
Oct 30, 2020
ContributorAuthor
I have resolved the conflicts. |
stoty
commented
Oct 30, 2020
Contributor
💔 -1 overall
This message was automatically generated. |
ChinmaySKulkarni
commented
Oct 30, 2020
Contributor
@richardantal can you please put up a patch for 4.x branch too? I will commit to both master and 4.x at the same time for consistency. Also, please squash all commits into 1. |
richardantalforce-pushed
the
PHOENIX-6083
branch
2 times, most recently
from
November 2, 2020 17:09
2a0ddfa to
8ef65eeComparerichardantal
commented
Nov 2, 2020
ContributorAuthor
Sure, thank you @ChinmaySKulkarni |
stoty
commented
Nov 2, 2020
Contributor
💔 -1 overall
This message was automatically generated. |
stoty
commented
Nov 2, 2020
Contributor
💔 -1 overall
This message was automatically generated. |
virajjasani
left a comment
Contributor
There was a problem hiding this comment.
Left small comments, looks good otherwise.
Uh oh!
There was an error while loading. Please reload this page.
| conn.createStatement().execute("CREATE VIEW " + fullViewName + " AS SELECT * FROM " + fullTableName); | ||
| conn.commit(); | ||
| final int[] exceptionCode = new int[1]; |
Contributor
There was a problem hiding this comment.
Not a strong opinion but since we want threads to mutate exceptionCode, maybe we can use AtomicInteger for exceptionCode rather than array with size 1, threads can set values and finally we can assert with assertEquals(exceptionCode.get(), 301) if you would prefer :)
ContributorAuthor
There was a problem hiding this comment.
exceptionCode is accessed from an inner class so has to be final, in order to be able to change its value from the other thread it has to be an array.
We can use AtomicInteger array but I am not sure if it adds any value.
Contributor
There was a problem hiding this comment.
I meant AtomicInteger as mutable final object similar to an array.
e.g
final AtomicInteger exceptionCode = new AtomicInteger();
&
} catch (SQLException e) {
exceptionCode.set(e.getErrorCode());
throw new RuntimeException(e);
} finally {
doneSignal.countDown();
}
&
assertEquals(exceptionCode.get(), 301);
Thought?
ContributorAuthor
There was a problem hiding this comment.
Yeah I think this a better solution. Nice one :)
Thanks for the review @virajjasani I have updated the commit.
richardantalforce-pushed
the
PHOENIX-6083
branch
from
November 3, 2020 16:48
8ef65ee to
291d4d5Comparerichardantalforce-pushed
the
PHOENIX-6083
branch
from
November 3, 2020 17:51
291d4d5 to
d1403ccComparestoty
commented
Nov 3, 2020
Contributor
💔 -1 overall
This message was automatically generated. |
stoty
commented
Nov 3, 2020
Contributor
💔 -1 overall
This message was automatically generated. |
ChinmaySKulkarni
commented
Nov 5, 2020
Contributor
Thanks @richardantal |
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.
…ow key