You are viewing a plain text version of this content. The canonical link for it is here.
Posted to common-issues@hadoop.apache.org by GitBox <gi...@apache.org> on 2020/12/08 22:42:39 UTC
[GitHub] [hadoop] jbrennan333 commented on a change in pull request #2521: HDFS-15711. Add Metrics to HttpFS Server.
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org