jbrennan333 commented on a change in pull request #2521:
URL: https://github.com/apache/hadoop/pull/2521#discussion_r538846892
##########
File path:
hadoop-hdfs-project/hadoop-hdfs-httpfs/src/main/java/org/apache/hadoop/fs/http/server/HttpFSServerWebApp.java
##########
@@ -123,6 +143,14 @@ public static HttpFSServerWebApp get() {
return SERVER;
}
+ /**
+ * gets the HttpFSServerMetrics instance.
+ * @return the HttpFSServerMetrics singleton.
+ */
+ public static HttpFSServerMetrics getMetrics() {
+ return metrics;
+ }
+
/**
* Returns HttpFSServer admin group.
*
Review comment:
I think you need to increment OpsCheckAccess() in FSAccess.execute().
##########
File path:
hadoop-hdfs-project/hadoop-hdfs-httpfs/src/main/java/org/apache/hadoop/fs/http/server/HttpFSServerWebApp.java
##########
@@ -110,9 +116,23 @@ public void init() throws ServerException {
@Override
public void destroy() {
SERVER = null;
+ if (metrics != null) {
+ metrics.shutdown();
+ }
super.destroy();
}
+ public static void setMetrics(Configuration config) {
Review comment:
Can you make this private? It is only used by init() in this class.
##########
File path:
hadoop-hdfs-project/hadoop-hdfs-httpfs/src/main/java/org/apache/hadoop/fs/http/server/HttpFSServerWebApp.java
##########
@@ -110,9 +116,23 @@ public void init() throws ServerException {
@Override
public void destroy() {
SERVER = null;
+ if (metrics != null) {
+ metrics.shutdown();
+ }
super.destroy();
}
+ public static void setMetrics(Configuration config) {
+ LOG.info("Initializing HttpFSServerMetrics");
+ HttpFSServerWebApp.metrics =
Review comment:
Can't you just use metrics instead of HttpFSServerWebApp.metrics? Same
a few lines below.
##########
File path:
hadoop-hdfs-project/hadoop-hdfs-httpfs/src/test/java/org/apache/hadoop/fs/http/server/TestHttpFSServer.java
##########
@@ -542,8 +572,12 @@ private void createDirWithHttp(String dirname, String
perms,
conn.setRequestMethod("PUT");
conn.connect();
Assert.assertEquals(HttpURLConnection.HTTP_OK, conn.getResponseCode());
+ Assert.assertEquals(1 + oldOpsMkdir,
+ HttpFSServerWebApp.get().getMetrics().getOpsMkdir());
}
+
+
Review comment:
Seems like extra blank lines were added by accident.
##########
File path:
hadoop-hdfs-project/hadoop-hdfs-httpfs/src/test/java/org/apache/hadoop/fs/http/server/TestHttpFSServer.java
##########
@@ -120,6 +122,25 @@
*/
public class TestHttpFSServer extends HFSTestCase {
+ /**
+ * define metric getters for unit tests.
+ */
+ private static Callable<Long> defaultEntryMetricGetter = () -> 0L;
+ private static Callable<Long> defaultExitMetricGetter = () -> 1L;
+ private static HashMap<String, Callable<Long>> metricsGetter =
+ new HashMap<String, Callable<Long>>() {
+ {
+ put("LISTSTATUS",
+ () -> HttpFSServerWebApp.get().getMetrics().getOpsListing());
+ put("MKDIRS",
+ () -> HttpFSServerWebApp.get().getMetrics().getOpsMkdir());
+ put("GETFILESTATUS",
+ () -> HttpFSServerWebApp.get().getMetrics().getOpsStat());
+ }
+ };
+
Review comment:
It's not clear to me why we need this metricsGetter. Seems to add
complexity with no added value. Can't you just call the appropriate
HttpFSServerWebApp.get().getMetrics().get* function wherever this is used?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
