HBASE-24456 : Create ImmutableScan and use it for CustomizedScanInfoBuilder #1818
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
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
virajjasani
changed the title
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
Apache9
requested changes
Jun 1, 2020
hbase-client/src/main/java/org/apache/hadoop/hbase/client/ImmutableScan.java
Outdated
Show resolved
Hide resolved
hbase-client/src/main/java/org/apache/hadoop/hbase/client/ImmutableScan.java
Outdated
Show resolved
Hide resolved
hbase-client/src/main/java/org/apache/hadoop/hbase/client/ImmutableScan.java
Outdated
Show resolved
Hide resolved
hbase-client/src/main/java/org/apache/hadoop/hbase/client/ImmutableScan.java
Outdated
Show resolved
Hide resolved
| @@ -164,7 +164,11 @@ public int getReplicaId() { | |||
| * @param level IsolationLevel for this query | |||
| */ | |||
| public Query setIsolationLevel(IsolationLevel level) { | |||
| setAttribute(ISOLATION_LEVEL, level.toBytes()); | |||
| if (this instanceof ImmutableScan) { | |||
There was a problem hiding this comment.
Because while constructing ImmutableScan(Scan), it calls parent Scan(Scan), which calls super.setIsolationLevel() and here, it calls setAttribute() which is overridden by ImmutableScan and hence, it will fail. I know this is not good check, but we do need to directly call super.setAttribute(), this.setAttribute() goes to ImmutableScan's setAttribute().
| if (cols != null && cols.size() > 0) { | ||
| for (byte[] col : cols) { | ||
| addColumn(fam, col); | ||
| if (!(this instanceof ImmutableScan)) { |
There was a problem hiding this comment.
This is not a good practise... Cna we avoid this instanceof for testing a sub class in the parent class?
There was a problem hiding this comment.
I really tried for this but didn't find better solution. For any Immutable child class, it will have to call super constructor, and here in Scan constructor, it calls setter methods, so the control will go to child class's overridden setter methods, which will fail :(
|
🎊 +1 overall
This message was automatically generated. |
|
@Apache9 I found a way without updating |
anoopsjohn
reviewed
Jun 1, 2020
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/CustomizedScanInfoBuilder.java
Outdated
Show resolved
Hide resolved
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
@Apache9 Please take a look when you get time. |
anoopsjohn
reviewed
Jun 2, 2020
| /** | ||
| * Create Immutable instance of Scan | ||
| */ | ||
| public ImmutableScan() { |
There was a problem hiding this comment.
When it is delegate model, better get the delegate via constructor arg always. Let the creator pass a new Scan() to this explicitly. Pls avoid this constructor.
There was a problem hiding this comment.
Sure, let me handle this.
anoopsjohn
reviewed
Jun 2, 2020
| * @throws IOException From parent constructor | ||
| */ | ||
| public ImmutableScan(Scan scan) throws IOException { | ||
| this.delegateScan = new Scan(scan); |
There was a problem hiding this comment.
As we are not exposing the original Scan directly, why to create a new Scan again here? No need
| * | ||
| * @param get Get to model Scan after | ||
| */ | ||
| public ImmutableScan(Get get) { |
There was a problem hiding this comment.
As of now we dont have this constructor req right? If not better not add now. When there is a need, we can add. At the place where we create this ImmutableScan, already the Get would have been converted to scan
hbase-client/src/main/java/org/apache/hadoop/hbase/client/ImmutableScan.java
Show resolved
Hide resolved
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
@Apache9 @anoopsjohn I have done changes as per your latest suggestions. Please take a look. |
anoopsjohn
reviewed
Jun 3, 2020
| * @param scan Copy all values from Scan | ||
| * @throws IOException From parent constructor | ||
| */ | ||
| public ImmutableScan(Scan scan) throws IOException { |
There was a problem hiding this comment.
Now we dont have to keep this throws IOE at this constructor. So below catch stuff in CustomizedScanInfoBuilder also can be avoided.
There was a problem hiding this comment.
Ohh, what a miss :(
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
anoopsjohn
approved these changes
Jun 4, 2020
virajjasani
added a commit
that referenced
this pull request
Jun 4, 2020
…uilder (#1818) Signed-off-by: Anoop Sam John <anoopsamjohn@apache.org> Signed-off-by: Duo Zhang <zhangduo@apache.org>
clarax
pushed a commit
to clarax/hbase
that referenced
this pull request
Nov 15, 2020
…uilder (apache#1818) Signed-off-by: Anoop Sam John <anoopsamjohn@apache.org> Signed-off-by: Duo Zhang <zhangduo@apache.org>
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.
No description provided.