-
Notifications
You must be signed in to change notification settings - Fork 8.9k
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
HADOOP-18302. Remove WhiteBox in hadoop-common module. #4457
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
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.
@aajisaka Thanks for your suggestion, I will refactor the code. |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
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.
When adding getter and setter, use limited scope as possible. You should consider in the order package-private -> protected -> public. In addition, would you add @VisibleForTesting
to specify it is used only from tests?
@aajisaka Thank you for helping to review the code, I will fix it as soon as possible. |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
@aajisaka Please help me to review the code again, thank you very much! |
🎊 +1 overall
This message was automatically generated. |
@aajisaka Please help me to review the code again, thank you very much! |
@@ -208,4 +209,8 @@ public void exceptionCaught(ChannelHandlerContext ctx, Throwable t) { | |||
LOG.warn("Encountered ", t); | |||
ctx.channel().close(); | |||
} | |||
|
|||
public Map<String, PortmapMapping> getMap() { |
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.
Should be package-private and annotated with @VisibleForTesting
.
...p-common-project/hadoop-common/src/main/java/org/apache/hadoop/metrics2/sink/StatsDSink.java
Show resolved
Hide resolved
public Graphite getGraphite() { | ||
return graphite; | ||
} | ||
|
||
public void setGraphite(Graphite graphite) { | ||
this.graphite = graphite; | ||
} |
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.
Thank you @slfan1989, but I disagree with replicating the class because we have to manually update the test class if there is any change in the original class. Instead, can we make MetricsRecordImpl.java public? Reference:
Lines 34 to 36 in e103c83
@InterfaceAudience.Private | |
@VisibleForTesting | |
public class MetricsCollectorImpl implements MetricsCollector, |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
@aajisaka Can you help review this pr again? Thank you very much! |
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'm +1 if the below comment is addressed. Thank you for the update, @slfan1989
...n-project/hadoop-common/src/main/java/org/apache/hadoop/metrics2/impl/MetricsRecordImpl.java
Show resolved
Hide resolved
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
@aajisaka Please help to review the code again, thank you very much! |
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
Merged into trunk. Thank you so much for your contribution, @slfan1989 ! |
@aajisaka Thank you very much for helping to review the code! |
Signed-off-by: Akira Ajisaka <aajisaka@apache.org>
JIRA:HADOOP-18302. Remove WhiteBox in hadoop-common module.
In HADOOP-18277 Jira, the removal of Whitebox was discussed,
aajisaka suggested that the relevant code needs to be refactored to completely remove the Whitebox class, I tried to refactor the code in this pr.