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
HBASE-26490 Add builder for class ReplicationLoadSink #3883
Conversation
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
@@ -22,7 +22,6 @@ | |||
private final long timestampStarted; | |||
private final long totalOpsProcessed; | |||
|
|||
// TODO: add the builder for this class | |||
@InterfaceAudience.Private | |||
public ReplicationLoadSink(long age, long timestamp, long timestampStarted, |
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.
The constructor should be private when you use Builder pattern.
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.
Right, will change it to private in the latest commit.
@@ -2836,10 +2836,12 @@ public static void mergeFrom(Message.Builder builder, CodedInputStream codedInpu | |||
|
|||
public static ReplicationLoadSink toReplicationLoadSink( | |||
ClusterStatusProtos.ReplicationLoadSink rls) { | |||
return new ReplicationLoadSink(rls.getAgeOfLastAppliedOp(), |
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.
Have you replaced all? only here?
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 checked the code and changed the constructor function to private. There is no problem with compiling. Here is the only invoke.
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.
what about codes in unit tests.
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.
No explicit invoke in UT. But I ran all the UTs in the same module. They all passed.
🎊 +1 overall
This message was automatically generated. |
🎊 +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 lgtm. Let's wait if there are other reviews.
@@ -22,9 +22,8 @@ | |||
private final long timestampStarted; | |||
private final long totalOpsProcessed; | |||
|
|||
// TODO: add the builder for this class | |||
@InterfaceAudience.Private |
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.
Since this is private now, we do not need IA.Private annotation then?
hbase-client/src/main/java/org/apache/hadoop/hbase/replication/ReplicationLoadSink.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. |
Signed-off-by: Reid Chan <reidchan@apache.org> Signed-off-by: Duo Zhang <zhangduo@apache.org>
Signed-off-by: Reid Chan <reidchan@apache.org> Signed-off-by: Duo Zhang <zhangduo@apache.org>
Signed-off-by: Reid Chan <reidchan@apache.org> Signed-off-by: Duo Zhang <zhangduo@apache.org>
https://issues.apache.org/jira/browse/HBASE-26490