PHOENIX-4864 Fix NullPointerException while Logging some DDL Statements #336
Conversation
This file contains 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
karanmehta93
requested changes
Aug 24, 2018
| @@ -45,7 +45,9 @@ public ResultSet executeQuery() throws SQLException { | |||
|
|
|||
| @Override | |||
| public ResultSet getResultSet() throws SQLException { | |||
| return new LoggingPhoenixResultSet(super.getResultSet(), phoenixMetricsLog, sql); | |||
| ResultSet resultSet = super.getResultSet(); | |||
| return (resultSet == null) ? null : new LoggingPhoenixResultSet(super.getResultSet(), | |||
There was a problem hiding this comment.
This is still a bug. super.getResultSet() call is not idempotent. You should replace this with resultSet local variable
ashuparekh
force-pushed
the
4.x-HBase-1.4
branch
2 times, most recently
from
8ab0d00
to
f2e3fdd
Compare
|
Added the corresponding test case. @vincentpoon @karanmehta93 |
| mutationWriteMetricsMap.size() == 0); | ||
| assertTrue("Mutation read metrics are not logged again.", | ||
| mutationReadMetricsMap.size() == 0); | ||
| assertTrue("Mutation write metrics are not logged again.", mutationWriteMetricsMap.size() |
There was a problem hiding this comment.
nit: unnecessary diff (in lot of places)
| @@ -32,6 +35,7 @@ | |||
|
|
|||
| import static org.junit.Assert.assertTrue; | |||
|
|
|||
|
|
|||
| @@ -45,7 +45,9 @@ public ResultSet executeQuery() throws SQLException { | |||
|
|
|||
| @Override | |||
| public ResultSet getResultSet() throws SQLException { | |||
| return new LoggingPhoenixResultSet(super.getResultSet(), phoenixMetricsLog, sql); | |||
| ResultSet resultSet = super.getResultSet(); | |||
There was a problem hiding this comment.
nit: add a comment that call to getResultSet() is not idempotent. Hence we cache and return accordingly.
|
Nice test coverage @ashuparekh |
ashuparekh
force-pushed
the
4.x-HBase-1.4
branch
from
f2e3fdd
to
e43db87
Compare
|
@karanmehta93 Incorporated the above suggestions |
ashuparekh
force-pushed
the
4.x-HBase-1.4
branch
from
e43db87
to
e498425
Compare
karanmehta93
approved these changes
Aug 24, 2018
|
Already merged. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Fixed the error. Still to add corresponding test case. @vincentpoon @karanmehta93