aajisaka commented on code in PR #4457:
URL: https://github.com/apache/hadoop/pull/4457#discussion_r946542663
##########
hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/ipc/TestRPC.java:
##########
@@ -1219,9 +1218,8 @@ public void testClientBackOff() throws Exception {
server = setupTestServer(builder);
@SuppressWarnings("unchecked")
Review Comment:
We can remove `@SuppressWarnings("unchecked")` here
##########
hadoop-common-project/hadoop-nfs/src/main/java/org/apache/hadoop/portmap/RpcProgramPortmap.java:
##########
@@ -208,4 +208,8 @@ public void exceptionCaught(ChannelHandlerContext ctx,
Throwable t) {
LOG.warn("Encountered ", t);
ctx.channel().close();
}
+
+ public ConcurrentHashMap<String, PortmapMapping> getMap() {
+ return map;
+ }
Review Comment:
This method can be package-private and we should use more abstract return
type (in this case, `Map`).
##########
hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/http/TestHttpServer.java:
##########
@@ -740,8 +738,7 @@ public void testBacklogSize() throws Exception
Configuration conf = new Configuration();
conf.setInt(HttpServer2.HTTP_SOCKET_BACKLOG_SIZE_KEY, backlogSize);
HttpServer2 srv = createServer("test", conf);
- List<?> listeners = (List<?>) Whitebox.getInternalState(srv,
- "listeners");
+ List<?> listeners = srv.getListeners();
ServerConnector listener = (ServerConnector)listeners.get(0);
Review Comment:
The same as above
##########
hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/metrics2/impl/TestStatsDMetrics.java:
##########
@@ -31,13 +31,13 @@
import java.util.List;
import java.util.Set;
+import org.apache.commons.lang3.reflect.FieldUtils;
Review Comment:
We should use getter and setter instead of relying on the FieldUtils.
##########
hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/ipc/TestIPC.java:
##########
@@ -881,9 +880,8 @@ private void checkBlocking(int readers, int readerQ, int
callQ) throws Exception
// start server
final TestServerQueue server =
new TestServerQueue(clients, readers, callQ, handlers, conf);
- CallQueueManager<Call> spy = spy(
- (CallQueueManager<Call>)Whitebox.getInternalState(server,
"callQueue"));
- Whitebox.setInternalState(server, "callQueue", spy);
+ CallQueueManager<Call> spy = spy(server.getCallQueue());
+ server.setCallQueue(spy);
Review Comment:
We can remove `@SuppressWarnings("unchecked")` from this method.
##########
hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/ipc/TestRPC.java:
##########
@@ -1282,9 +1280,8 @@ public void testClientBackOffByResponseTime() throws
Exception {
Server server = setupDecayRpcSchedulerandTestServer(ns + ".");
@SuppressWarnings("unchecked")
Review Comment:
The same as above
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/metrics2/sink/GraphiteSink.java:
##########
@@ -37,171 +37,176 @@
import java.nio.charset.StandardCharsets;
/**
- * A metrics sink that writes to a Graphite server
+ * A metrics sink that writes to a Graphite server.
*/
@InterfaceAudience.Public
@InterfaceStability.Evolving
public class GraphiteSink implements MetricsSink, Closeable {
- private static final Logger LOG =
- LoggerFactory.getLogger(GraphiteSink.class);
- private static final String SERVER_HOST_KEY = "server_host";
- private static final String SERVER_PORT_KEY = "server_port";
- private static final String METRICS_PREFIX = "metrics_prefix";
- private String metricsPrefix = null;
- private Graphite graphite = null;
-
- @Override
- public void init(SubsetConfiguration conf) {
- // Get Graphite host configurations.
- final String serverHost = conf.getString(SERVER_HOST_KEY);
- final int serverPort =
Integer.parseInt(conf.getString(SERVER_PORT_KEY));
-
- // Get Graphite metrics graph prefix.
- metricsPrefix = conf.getString(METRICS_PREFIX);
- if (metricsPrefix == null)
- metricsPrefix = "";
-
- graphite = new Graphite(serverHost, serverPort);
- graphite.connect();
+ private static final Logger LOG =
+ LoggerFactory.getLogger(GraphiteSink.class);
+ private static final String SERVER_HOST_KEY = "server_host";
+ private static final String SERVER_PORT_KEY = "server_port";
+ private static final String METRICS_PREFIX = "metrics_prefix";
+ private String metricsPrefix = null;
+ private Graphite graphite = null;
+
+ @Override
+ public void init(SubsetConfiguration conf) {
+ // Get Graphite host configurations.
+ final String serverHost = conf.getString(SERVER_HOST_KEY);
+ final int serverPort = Integer.parseInt(conf.getString(SERVER_PORT_KEY));
+
+ // Get Graphite metrics graph prefix.
+ metricsPrefix = conf.getString(METRICS_PREFIX);
+ if (metricsPrefix == null) {
+ metricsPrefix = "";
}
- @Override
- public void putMetrics(MetricsRecord record) {
- StringBuilder lines = new StringBuilder();
- StringBuilder metricsPathPrefix = new StringBuilder();
-
- // Configure the hierarchical place to display the graph.
- metricsPathPrefix.append(metricsPrefix).append(".")
- .append(record.context()).append(".").append(record.name());
-
- for (MetricsTag tag : record.tags()) {
- if (tag.value() != null) {
- metricsPathPrefix.append(".")
- .append(tag.name())
- .append("=")
- .append(tag.value());
- }
- }
-
- // The record timestamp is in milliseconds while Graphite expects an
epoc time in seconds.
- long timestamp = record.timestamp() / 1000L;
+ graphite = new Graphite(serverHost, serverPort);
+ graphite.connect();
+ }
+
+ @Override
+ public void putMetrics(MetricsRecord record) {
+ StringBuilder lines = new StringBuilder();
+ StringBuilder metricsPathPrefix = new StringBuilder();
+
+ // Configure the hierarchical place to display the graph.
+ metricsPathPrefix.append(metricsPrefix).append(".")
+ .append(record.context()).append(".").append(record.name());
+
+ for (MetricsTag tag : record.tags()) {
+ if (tag.value() != null) {
+ metricsPathPrefix.append(".")
+ .append(tag.name())
+ .append("=")
+ .append(tag.value());
+ }
+ }
- // Collect datapoints.
- for (AbstractMetric metric : record.metrics()) {
- lines.append(
- metricsPathPrefix.toString() + "."
- + metric.name().replace(' ', '.')).append(" ")
- .append(metric.value()).append(" ").append(timestamp)
- .append("\n");
- }
+ // The record timestamp is in milliseconds while Graphite expects an epoc
time in seconds.
+ long timestamp = record.timestamp() / 1000L;
- try {
- graphite.write(lines.toString());
- } catch (Exception e) {
- LOG.warn("Error sending metrics to Graphite", e);
- try {
- graphite.close();
- } catch (Exception e1) {
- throw new MetricsException("Error closing connection to Graphite",
e1);
- }
- }
+ // Collect datapoints.
+ for (AbstractMetric metric : record.metrics()) {
+ lines.append(metricsPathPrefix + "." + metric.name().replace(' ',
'.')).append(" ")
+ .append(metric.value()).append(" ").append(timestamp)
+ .append("\n");
}
- @Override
- public void flush() {
+ try {
+ graphite.write(lines.toString());
+ } catch (Exception e) {
+ LOG.warn("Error sending metrics to Graphite.", e);
try {
- graphite.flush();
- } catch (Exception e) {
- LOG.warn("Error flushing metrics to Graphite", e);
- try {
- graphite.close();
- } catch (Exception e1) {
- throw new MetricsException("Error closing connection to Graphite",
e1);
- }
+ graphite.close();
+ } catch (Exception e1) {
+ throw new MetricsException("Error closing connection to Graphite", e1);
}
}
-
- @Override
- public void close() throws IOException {
- graphite.close();
+ }
+
+ @Override
+ public void flush() {
+ try {
+ graphite.flush();
+ } catch (Exception e) {
+ LOG.warn("Error flushing metrics to Graphite.", e);
+ try {
+ graphite.close();
+ } catch (Exception e1) {
+ throw new MetricsException("Error closing connection to Graphite.",
e1);
+ }
}
+ }
- public static class Graphite {
- private final static int MAX_CONNECTION_FAILURES = 5;
+ @Override
+ public void close() throws IOException {
+ graphite.close();
+ }
- private String serverHost;
- private int serverPort;
- private Writer writer = null;
- private Socket socket = null;
- private int connectionFailures = 0;
+ public static class Graphite {
+ private final static int MAX_CONNECTION_FAILURES = 5;
- public Graphite(String serverHost, int serverPort) {
- this.serverHost = serverHost;
- this.serverPort = serverPort;
- }
+ private String serverHost;
+ private int serverPort;
+ private Writer writer = null;
+ private Socket socket = null;
+ private int connectionFailures = 0;
- public void connect() {
- if (isConnected()) {
- throw new MetricsException("Already connected to Graphite");
- }
- if (tooManyConnectionFailures()) {
- // return silently (there was ERROR in logs when we reached limit
for the first time)
- return;
- }
- try {
+ public Graphite(String serverHost, int serverPort) {
+ this.serverHost = serverHost;
+ this.serverPort = serverPort;
+ }
+
+ public void connect() {
+ if (isConnected()) {
+ throw new MetricsException("Already connected to Graphite");
+ }
+ if (tooManyConnectionFailures()) {
+ // return silently (there was ERROR in logs when we reached limit for
the first time)
+ return;
+ }
+ try {
// Open a connection to Graphite server.
- socket = new Socket(serverHost, serverPort);
+ socket = new Socket(serverHost, serverPort);
writer = new OutputStreamWriter(socket.getOutputStream(),
StandardCharsets.UTF_8);
- } catch (Exception e) {
- connectionFailures++;
- if (tooManyConnectionFailures()) {
- // first time when connection limit reached, report to logs
- LOG.error("Too many connection failures, would not try to connect
again.");
- }
- throw new MetricsException("Error creating connection, "
- + serverHost + ":" + serverPort, e);
+ } catch (Exception e) {
+ connectionFailures++;
+ if (tooManyConnectionFailures()) {
+ // first time when connection limit reached, report to logs
+ LOG.error("Too many connection failures, would not try to connect
again.");
}
+ throw new MetricsException("Error creating connection, " +
+ serverHost + ":" + serverPort, e);
}
+ }
- public void write(String msg) throws IOException {
- if (!isConnected()) {
- connect();
- }
- if (isConnected()) {
- writer.write(msg);
- }
+ public void write(String msg) throws IOException {
+ if (!isConnected()) {
+ connect();
}
-
- public void flush() throws IOException {
- if (isConnected()) {
- writer.flush();
- }
+ if (isConnected()) {
+ writer.write(msg);
}
+ }
- public boolean isConnected() {
- return socket != null && socket.isConnected() && !socket.isClosed();
+ public void flush() throws IOException {
+ if (isConnected()) {
+ writer.flush();
}
+ }
- public void close() throws IOException {
- try {
- if (writer != null) {
- writer.close();
- }
- } catch (IOException ex) {
- if (socket != null) {
- socket.close();
- }
- } finally {
- socket = null;
- writer = null;
- }
- }
+ public boolean isConnected() {
+ return socket != null && socket.isConnected() && !socket.isClosed();
+ }
- private boolean tooManyConnectionFailures() {
- return connectionFailures > MAX_CONNECTION_FAILURES;
+ public void close() throws IOException {
+ try {
+ if (writer != null) {
+ writer.close();
+ }
+ } catch (IOException ex) {
+ if (socket != null) {
+ socket.close();
+ }
+ } finally {
+ socket = null;
+ writer = null;
}
+ }
+ private boolean tooManyConnectionFailures() {
+ return connectionFailures > MAX_CONNECTION_FAILURES;
}
+ }
+
+ public Graphite getGraphite() {
+ return graphite;
+ }
+ public void setGraphite(Graphite graphite) {
+ this.graphite = graphite;
+ }
Review Comment:
The methods are too public. Could you move the package of
`TestGraphiteMetrics` to org.apache.hadoop.metrics2.sink? That way we can make
the methods package-private.
##########
hadoop-common-project/hadoop-nfs/src/test/java/org/apache/hadoop/portmap/TestPortmap.java:
##########
@@ -76,7 +75,7 @@ public void testIdle() throws InterruptedException,
IOException {
}
@Test(timeout = 10000)
- public void testRegistration() throws IOException, InterruptedException {
+ public void testRegistration() throws IOException, InterruptedException,
IllegalAccessException {
Review Comment:
I don't think this change is needed.
##########
hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/http/TestHttpServer.java:
##########
@@ -663,8 +662,7 @@ private HttpServer2 checkBindAddress(String host, int port,
boolean findPort)
HttpServer2 server = createServer(host, port);
try {
// not bound, ephemeral should return requested port (0 for ephemeral)
- List<?> listeners = (List<?>) Whitebox.getInternalState(server,
- "listeners");
+ List<?> listeners = server.getListeners();
ServerConnector listener = (ServerConnector)listeners.get(0);
Review Comment:
We can use `List<ServerConnector>` instead of `List<?>` and then we can
remove the casting `(ServerConnector)`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
