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 2022/10/14 05:57:01 UTC

[GitHub] [hadoop] slfan1989 opened a new pull request, #5030: YARN-11345. [Federation] Refactoring Yarn Router's Application Web Page.

slfan1989 opened a new pull request, #5030:
URL: https://github.com/apache/hadoop/pull/5030

   JIRA: YARN-11345.  [Federation] Refactoring Yarn Router's Application Web Page.


-- 
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: common-issues-unsubscribe@hadoop.apache.org

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


[GitHub] [hadoop] goiri commented on a diff in pull request #5030: YARN-11345. [Federation] Refactoring Yarn Router's Application Web Page.

Posted by GitBox <gi...@apache.org>.
goiri commented on code in PR #5030:
URL: https://github.com/apache/hadoop/pull/5030#discussion_r1001069884


##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-router/src/main/java/org/apache/hadoop/yarn/server/router/webapp/AppsBlock.java:
##########
@@ -81,53 +155,57 @@ protected void render(Block html) {
 
     // Render the applications
     StringBuilder appsTableData = new StringBuilder("[\n");
-    for (AppInfo app : apps.getApps()) {
-      try {
-
-        String percent = String.format("%.1f", app.getProgress() * 100.0F);
-        String trackingURL =
-            app.getTrackingUrl() == null ? "#" : app.getTrackingUrl();
-        // AppID numerical value parsed by parseHadoopID in yarn.dt.plugins.js
-        appsTableData.append("[\"")
-            .append("<a href='").append(trackingURL).append("'>")
-            .append(app.getAppId()).append("</a>\",\"")
-            .append(escape(app.getUser())).append("\",\"")
-            .append(escape(app.getName())).append("\",\"")
-            .append(escape(app.getApplicationType())).append("\",\"")
-            .append(escape(app.getQueue())).append("\",\"")
-            .append(String.valueOf(app.getPriority())).append("\",\"")
-            .append(app.getStartTime()).append("\",\"")
-            .append(app.getFinishTime()).append("\",\"")
-            .append(app.getState()).append("\",\"")
-            .append(app.getFinalStatus()).append("\",\"")
-            // Progress bar
-            .append("<br title='").append(percent).append("'> <div class='")
-            .append(C_PROGRESSBAR).append("' title='")
-            .append(join(percent, '%')).append("'> ").append("<div class='")
-            .append(C_PROGRESSBAR_VALUE).append("' style='")
-            .append(join("width:", percent, '%')).append("'> </div> </div>")
-            // History link
-            .append("\",\"<a href='").append(trackingURL).append("'>")
-            .append("History").append("</a>");
-        appsTableData.append("\"],\n");
-
-      } catch (Exception e) {
-        LOG.info(
-            "Cannot add application {}: {}", app.getAppId(), e.getMessage());
+
+    if (appsInfo != null) {
+      Collection<AppInfo> apps = appsInfo.getApps();
+      if (CollectionUtils.isNotEmpty(apps)) {
+        int numApps = apps.size();
+        int i = 0;

Review Comment:
   i++ missing?



-- 
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: common-issues-unsubscribe@hadoop.apache.org

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


[GitHub] [hadoop] slfan1989 commented on a diff in pull request #5030: YARN-11345. [Federation] Refactoring Yarn Router's Application Web Page.

Posted by GitBox <gi...@apache.org>.
slfan1989 commented on code in PR #5030:
URL: https://github.com/apache/hadoop/pull/5030#discussion_r1001179567


##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-router/src/main/java/org/apache/hadoop/yarn/server/router/webapp/AppsBlock.java:
##########
@@ -81,53 +155,57 @@ protected void render(Block html) {
 
     // Render the applications
     StringBuilder appsTableData = new StringBuilder("[\n");
-    for (AppInfo app : apps.getApps()) {
-      try {
-
-        String percent = String.format("%.1f", app.getProgress() * 100.0F);
-        String trackingURL =
-            app.getTrackingUrl() == null ? "#" : app.getTrackingUrl();
-        // AppID numerical value parsed by parseHadoopID in yarn.dt.plugins.js
-        appsTableData.append("[\"")
-            .append("<a href='").append(trackingURL).append("'>")
-            .append(app.getAppId()).append("</a>\",\"")
-            .append(escape(app.getUser())).append("\",\"")
-            .append(escape(app.getName())).append("\",\"")
-            .append(escape(app.getApplicationType())).append("\",\"")
-            .append(escape(app.getQueue())).append("\",\"")
-            .append(String.valueOf(app.getPriority())).append("\",\"")
-            .append(app.getStartTime()).append("\",\"")
-            .append(app.getFinishTime()).append("\",\"")
-            .append(app.getState()).append("\",\"")
-            .append(app.getFinalStatus()).append("\",\"")
-            // Progress bar
-            .append("<br title='").append(percent).append("'> <div class='")
-            .append(C_PROGRESSBAR).append("' title='")
-            .append(join(percent, '%')).append("'> ").append("<div class='")
-            .append(C_PROGRESSBAR_VALUE).append("' style='")
-            .append(join("width:", percent, '%')).append("'> </div> </div>")
-            // History link
-            .append("\",\"<a href='").append(trackingURL).append("'>")
-            .append("History").append("</a>");
-        appsTableData.append("\"],\n");
-
-      } catch (Exception e) {
-        LOG.info(
-            "Cannot add application {}: {}", app.getAppId(), e.getMessage());
+
+    if (appsInfo != null) {
+      Collection<AppInfo> apps = appsInfo.getApps();
+      if (CollectionUtils.isNotEmpty(apps)) {
+        int numApps = apps.size();
+        int i = 0;
+        for (AppInfo app : apps) {
+          try {
+            String percent = String.format("%.1f", app.getProgress() * 100.0F);
+            String trackingURL =
+                app.getTrackingUrl() == null ? "#" : app.getTrackingUrl();
+
+            // AppID numerical value parsed by parseHadoopID in yarn.dt.plugins.js
+            appsTableData.append("[\"")
+                .append("<a href='").append(trackingURL).append("'>")
+                .append(app.getAppId()).append("</a>\",\"")
+                .append(escape(app.getUser())).append("\",\"")
+                .append(escape(app.getName())).append("\",\"")
+                .append(escape(app.getApplicationType())).append("\",\"")
+                .append(escape(app.getQueue())).append("\",\"")
+                .append(app.getPriority()).append("\",\"")
+                .append(app.getStartTime()).append("\",\"")
+                .append(app.getFinishTime()).append("\",\"")
+                .append(app.getState()).append("\",\"")
+                .append(app.getFinalStatus()).append("\",\"")
+                // Progress bar
+                .append("<br title='").append(percent).append("'> <div class='")
+                .append(C_PROGRESSBAR).append("' title='")
+                .append(join(percent, '%')).append("'> ").append("<div class='")
+                .append(C_PROGRESSBAR_VALUE).append("' style='")
+                .append(join("width:", percent, '%')).append("'> </div> </div>")
+                // History link
+                .append("\",\"<a href='").append(trackingURL).append("'>")
+                .append("History").append("</a>");
+            appsTableData.append("\"]\n");
+
+            if (i < numApps - 1) {
+              appsTableData.append(",");
+            }
+          } catch (Exception e) {
+            LOG.info("Cannot add application {}: {}", app.getAppId(), e.getMessage());
+          }
+          i++;

Review Comment:
   i++ is placed after try...catch.



-- 
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: common-issues-unsubscribe@hadoop.apache.org

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


[GitHub] [hadoop] goiri commented on a diff in pull request #5030: YARN-11345. [Federation] Refactoring Yarn Router's Application Web Page.

Posted by GitBox <gi...@apache.org>.
goiri commented on code in PR #5030:
URL: https://github.com/apache/hadoop/pull/5030#discussion_r999695420


##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-router/src/main/java/org/apache/hadoop/yarn/server/router/webapp/AppsBlock.java:
##########
@@ -81,53 +154,53 @@ protected void render(Block html) {
 
     // Render the applications
     StringBuilder appsTableData = new StringBuilder("[\n");
-    for (AppInfo app : apps.getApps()) {
-      try {
-
-        String percent = String.format("%.1f", app.getProgress() * 100.0F);
-        String trackingURL =
-            app.getTrackingUrl() == null ? "#" : app.getTrackingUrl();
-        // AppID numerical value parsed by parseHadoopID in yarn.dt.plugins.js
-        appsTableData.append("[\"")
-            .append("<a href='").append(trackingURL).append("'>")
-            .append(app.getAppId()).append("</a>\",\"")
-            .append(escape(app.getUser())).append("\",\"")
-            .append(escape(app.getName())).append("\",\"")
-            .append(escape(app.getApplicationType())).append("\",\"")
-            .append(escape(app.getQueue())).append("\",\"")
-            .append(String.valueOf(app.getPriority())).append("\",\"")
-            .append(app.getStartTime()).append("\",\"")
-            .append(app.getFinishTime()).append("\",\"")
-            .append(app.getState()).append("\",\"")
-            .append(app.getFinalStatus()).append("\",\"")
-            // Progress bar
-            .append("<br title='").append(percent).append("'> <div class='")
-            .append(C_PROGRESSBAR).append("' title='")
-            .append(join(percent, '%')).append("'> ").append("<div class='")
-            .append(C_PROGRESSBAR_VALUE).append("' style='")
-            .append(join("width:", percent, '%')).append("'> </div> </div>")
-            // History link
-            .append("\",\"<a href='").append(trackingURL).append("'>")
-            .append("History").append("</a>");
-        appsTableData.append("\"],\n");
-
-      } catch (Exception e) {
-        LOG.info(
-            "Cannot add application {}: {}", app.getAppId(), e.getMessage());
+
+    if (appsInfo != null && CollectionUtils.isNotEmpty(appsInfo.getApps())) {
+      for (AppInfo app : appsInfo.getApps()) {
+        try {
+
+          String percent = String.format("%.1f", app.getProgress() * 100.0F);
+          String trackingURL =
+              app.getTrackingUrl() == null ? "#" : app.getTrackingUrl();
+
+          // AppID numerical value parsed by parseHadoopID in yarn.dt.plugins.js
+          appsTableData.append("[\"")
+              .append("<a href='").append(trackingURL).append("'>")
+              .append(app.getAppId()).append("</a>\",\"")
+              .append(escape(app.getUser())).append("\",\"")
+              .append(escape(app.getName())).append("\",\"")
+              .append(escape(app.getApplicationType())).append("\",\"")
+              .append(escape(app.getQueue())).append("\",\"")
+              .append(app.getPriority()).append("\",\"")
+              .append(app.getStartTime()).append("\",\"")
+              .append(app.getFinishTime()).append("\",\"")
+              .append(app.getState()).append("\",\"")
+              .append(app.getFinalStatus()).append("\",\"")
+              // Progress bar
+              .append("<br title='").append(percent).append("'> <div class='")
+              .append(C_PROGRESSBAR).append("' title='")
+              .append(join(percent, '%')).append("'> ").append("<div class='")
+              .append(C_PROGRESSBAR_VALUE).append("' style='")
+              .append(join("width:", percent, '%')).append("'> </div> </div>")
+              // History link
+              .append("\",\"<a href='").append(trackingURL).append("'>")
+              .append("History").append("</a>");
+          appsTableData.append("\"],\n");
+
+        } catch (Exception e) {
+          LOG.info(
+                  "Cannot add application {}: {}", app.getAppId(), e.getMessage());
+        }
+      }
+      if (appsTableData.charAt(appsTableData.length() - 2) == ',') {

Review Comment:
   As we are refactoring, this is a little hacky.
   At least it could be more readable.



##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-router/src/main/java/org/apache/hadoop/yarn/server/router/webapp/AppsBlock.java:
##########
@@ -81,53 +154,53 @@ protected void render(Block html) {
 
     // Render the applications
     StringBuilder appsTableData = new StringBuilder("[\n");
-    for (AppInfo app : apps.getApps()) {
-      try {
-
-        String percent = String.format("%.1f", app.getProgress() * 100.0F);
-        String trackingURL =
-            app.getTrackingUrl() == null ? "#" : app.getTrackingUrl();
-        // AppID numerical value parsed by parseHadoopID in yarn.dt.plugins.js
-        appsTableData.append("[\"")
-            .append("<a href='").append(trackingURL).append("'>")
-            .append(app.getAppId()).append("</a>\",\"")
-            .append(escape(app.getUser())).append("\",\"")
-            .append(escape(app.getName())).append("\",\"")
-            .append(escape(app.getApplicationType())).append("\",\"")
-            .append(escape(app.getQueue())).append("\",\"")
-            .append(String.valueOf(app.getPriority())).append("\",\"")
-            .append(app.getStartTime()).append("\",\"")
-            .append(app.getFinishTime()).append("\",\"")
-            .append(app.getState()).append("\",\"")
-            .append(app.getFinalStatus()).append("\",\"")
-            // Progress bar
-            .append("<br title='").append(percent).append("'> <div class='")
-            .append(C_PROGRESSBAR).append("' title='")
-            .append(join(percent, '%')).append("'> ").append("<div class='")
-            .append(C_PROGRESSBAR_VALUE).append("' style='")
-            .append(join("width:", percent, '%')).append("'> </div> </div>")
-            // History link
-            .append("\",\"<a href='").append(trackingURL).append("'>")
-            .append("History").append("</a>");
-        appsTableData.append("\"],\n");
-
-      } catch (Exception e) {
-        LOG.info(
-            "Cannot add application {}: {}", app.getAppId(), e.getMessage());
+
+    if (appsInfo != null && CollectionUtils.isNotEmpty(appsInfo.getApps())) {
+      for (AppInfo app : appsInfo.getApps()) {
+        try {
+
+          String percent = String.format("%.1f", app.getProgress() * 100.0F);
+          String trackingURL =
+              app.getTrackingUrl() == null ? "#" : app.getTrackingUrl();
+
+          // AppID numerical value parsed by parseHadoopID in yarn.dt.plugins.js
+          appsTableData.append("[\"")
+              .append("<a href='").append(trackingURL).append("'>")
+              .append(app.getAppId()).append("</a>\",\"")
+              .append(escape(app.getUser())).append("\",\"")
+              .append(escape(app.getName())).append("\",\"")
+              .append(escape(app.getApplicationType())).append("\",\"")
+              .append(escape(app.getQueue())).append("\",\"")
+              .append(app.getPriority()).append("\",\"")
+              .append(app.getStartTime()).append("\",\"")
+              .append(app.getFinishTime()).append("\",\"")
+              .append(app.getState()).append("\",\"")
+              .append(app.getFinalStatus()).append("\",\"")
+              // Progress bar
+              .append("<br title='").append(percent).append("'> <div class='")
+              .append(C_PROGRESSBAR).append("' title='")
+              .append(join(percent, '%')).append("'> ").append("<div class='")
+              .append(C_PROGRESSBAR_VALUE).append("' style='")
+              .append(join("width:", percent, '%')).append("'> </div> </div>")
+              // History link
+              .append("\",\"<a href='").append(trackingURL).append("'>")
+              .append("History").append("</a>");
+          appsTableData.append("\"],\n");
+
+        } catch (Exception e) {
+          LOG.info(

Review Comment:
   One line?



-- 
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: common-issues-unsubscribe@hadoop.apache.org

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


[GitHub] [hadoop] hadoop-yetus commented on pull request #5030: YARN-11345. [Federation] Refactoring Yarn Router's Application Web Page.

Posted by GitBox <gi...@apache.org>.
hadoop-yetus commented on PR #5030:
URL: https://github.com/apache/hadoop/pull/5030#issuecomment-1286042680

   :confetti_ball: **+1 overall**
   
   
   
   
   
   
   | Vote | Subsystem | Runtime |  Logfile | Comment |
   |:----:|----------:|--------:|:--------:|:-------:|
   | +0 :ok: |  reexec  |   0m 52s |  |  Docker mode activated.  |
   |||| _ Prechecks _ |
   | +1 :green_heart: |  dupname  |   0m  0s |  |  No case conflicting files found.  |
   | +0 :ok: |  codespell  |   0m  0s |  |  codespell was not available.  |
   | +0 :ok: |  detsecrets  |   0m  0s |  |  detect-secrets was not available.  |
   | +1 :green_heart: |  @author  |   0m  0s |  |  The patch does not contain any @author tags.  |
   | +1 :green_heart: |  test4tests  |   0m  0s |  |  The patch appears to include 1 new or modified test files.  |
   |||| _ trunk Compile Tests _ |
   | +0 :ok: |  mvndep  |  15m 28s |  |  Maven dependency ordering for branch  |
   | +1 :green_heart: |  mvninstall  |  28m 33s |  |  trunk passed  |
   | +1 :green_heart: |  compile  |  10m 23s |  |  trunk passed with JDK Ubuntu-11.0.16+8-post-Ubuntu-0ubuntu120.04  |
   | +1 :green_heart: |  compile  |   8m 49s |  |  trunk passed with JDK Private Build-1.8.0_342-8u342-b07-0ubuntu1~20.04-b07  |
   | +1 :green_heart: |  checkstyle  |   1m 46s |  |  trunk passed  |
   | +1 :green_heart: |  mvnsite  |   1m 32s |  |  trunk passed  |
   | +1 :green_heart: |  javadoc  |   1m 35s |  |  trunk passed with JDK Ubuntu-11.0.16+8-post-Ubuntu-0ubuntu120.04  |
   | +1 :green_heart: |  javadoc  |   1m 23s |  |  trunk passed with JDK Private Build-1.8.0_342-8u342-b07-0ubuntu1~20.04-b07  |
   | +1 :green_heart: |  spotbugs  |   2m 58s |  |  trunk passed  |
   | +1 :green_heart: |  shadedclient  |  23m 25s |  |  branch has no errors when building and testing our client artifacts.  |
   |||| _ Patch Compile Tests _ |
   | +0 :ok: |  mvndep  |   0m 24s |  |  Maven dependency ordering for patch  |
   | +1 :green_heart: |  mvninstall  |   0m 59s |  |  the patch passed  |
   | +1 :green_heart: |  compile  |   9m 47s |  |  the patch passed with JDK Ubuntu-11.0.16+8-post-Ubuntu-0ubuntu120.04  |
   | +1 :green_heart: |  javac  |   9m 47s |  |  the patch passed  |
   | +1 :green_heart: |  compile  |   8m 45s |  |  the patch passed with JDK Private Build-1.8.0_342-8u342-b07-0ubuntu1~20.04-b07  |
   | +1 :green_heart: |  javac  |   8m 45s |  |  the patch passed  |
   | +1 :green_heart: |  blanks  |   0m  0s |  |  The patch has no blanks issues.  |
   | +1 :green_heart: |  checkstyle  |   1m 36s |  |  the patch passed  |
   | +1 :green_heart: |  mvnsite  |   1m 24s |  |  the patch passed  |
   | +1 :green_heart: |  javadoc  |   1m 18s |  |  the patch passed with JDK Ubuntu-11.0.16+8-post-Ubuntu-0ubuntu120.04  |
   | +1 :green_heart: |  javadoc  |   1m 18s |  |  the patch passed with JDK Private Build-1.8.0_342-8u342-b07-0ubuntu1~20.04-b07  |
   | +1 :green_heart: |  spotbugs  |   2m 59s |  |  the patch passed  |
   | +1 :green_heart: |  shadedclient  |  23m 35s |  |  patch has no errors when building and testing our client artifacts.  |
   |||| _ Other Tests _ |
   | +1 :green_heart: |  unit  |   4m 45s |  |  hadoop-yarn-common in the patch passed.  |
   | +1 :green_heart: |  unit  |   5m 21s |  |  hadoop-yarn-server-router in the patch passed.  |
   | +1 :green_heart: |  asflicense  |   0m 48s |  |  The patch does not generate ASF License warnings.  |
   |  |   | 162m 43s |  |  |
   
   
   | Subsystem | Report/Notes |
   |----------:|:-------------|
   | Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-5030/4/artifact/out/Dockerfile |
   | GITHUB PR | https://github.com/apache/hadoop/pull/5030 |
   | Optional Tests | dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient spotbugs checkstyle codespell detsecrets |
   | uname | Linux 5262eacd5120 4.15.0-192-generic #203-Ubuntu SMP Wed Aug 10 17:40:03 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux |
   | Build tool | maven |
   | Personality | dev-support/bin/hadoop.sh |
   | git revision | trunk / ca3270139a999c571b191194487a2fc118a19152 |
   | Default Java | Private Build-1.8.0_342-8u342-b07-0ubuntu1~20.04-b07 |
   | Multi-JDK versions | /usr/lib/jvm/java-11-openjdk-amd64:Ubuntu-11.0.16+8-post-Ubuntu-0ubuntu120.04 /usr/lib/jvm/java-8-openjdk-amd64:Private Build-1.8.0_342-8u342-b07-0ubuntu1~20.04-b07 |
   |  Test Results | https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-5030/4/testReport/ |
   | Max. process+thread count | 753 (vs. ulimit of 5500) |
   | modules | C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-router U: hadoop-yarn-project/hadoop-yarn |
   | Console output | https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-5030/4/console |
   | versions | git=2.25.1 maven=3.6.3 spotbugs=4.2.2 |
   | Powered by | Apache Yetus 0.14.0 https://yetus.apache.org |
   
   
   This message was automatically generated.
   
   


-- 
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: common-issues-unsubscribe@hadoop.apache.org

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


[GitHub] [hadoop] slfan1989 commented on pull request #5030: YARN-11345. [Federation] Refactoring Yarn Router's Application Web Page.

Posted by GitBox <gi...@apache.org>.
slfan1989 commented on PR #5030:
URL: https://github.com/apache/hadoop/pull/5030#issuecomment-1279167212

   @goiri Can you help review the code? Thank you very much!


-- 
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: common-issues-unsubscribe@hadoop.apache.org

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


[GitHub] [hadoop] hadoop-yetus commented on pull request #5030: YARN-11345. [Federation] Refactoring Yarn Router's Application Web Page.

Posted by GitBox <gi...@apache.org>.
hadoop-yetus commented on PR #5030:
URL: https://github.com/apache/hadoop/pull/5030#issuecomment-1287294572

   :confetti_ball: **+1 overall**
   
   
   
   
   
   
   | Vote | Subsystem | Runtime |  Logfile | Comment |
   |:----:|----------:|--------:|:--------:|:-------:|
   | +0 :ok: |  reexec  |   0m 52s |  |  Docker mode activated.  |
   |||| _ Prechecks _ |
   | +1 :green_heart: |  dupname  |   0m  0s |  |  No case conflicting files found.  |
   | +0 :ok: |  codespell  |   0m  0s |  |  codespell was not available.  |
   | +0 :ok: |  detsecrets  |   0m  0s |  |  detect-secrets was not available.  |
   | +1 :green_heart: |  @author  |   0m  0s |  |  The patch does not contain any @author tags.  |
   | +1 :green_heart: |  test4tests  |   0m  0s |  |  The patch appears to include 1 new or modified test files.  |
   |||| _ trunk Compile Tests _ |
   | +0 :ok: |  mvndep  |  15m 12s |  |  Maven dependency ordering for branch  |
   | +1 :green_heart: |  mvninstall  |  29m  1s |  |  trunk passed  |
   | +1 :green_heart: |  compile  |  10m 38s |  |  trunk passed with JDK Ubuntu-11.0.16+8-post-Ubuntu-0ubuntu120.04  |
   | +1 :green_heart: |  compile  |   9m 10s |  |  trunk passed with JDK Private Build-1.8.0_342-8u342-b07-0ubuntu1~20.04-b07  |
   | +1 :green_heart: |  checkstyle  |   2m  5s |  |  trunk passed  |
   | +1 :green_heart: |  mvnsite  |   2m  3s |  |  trunk passed  |
   | +1 :green_heart: |  javadoc  |   2m  2s |  |  trunk passed with JDK Ubuntu-11.0.16+8-post-Ubuntu-0ubuntu120.04  |
   | +1 :green_heart: |  javadoc  |   1m 53s |  |  trunk passed with JDK Private Build-1.8.0_342-8u342-b07-0ubuntu1~20.04-b07  |
   | +1 :green_heart: |  spotbugs  |   3m 24s |  |  trunk passed  |
   | +1 :green_heart: |  shadedclient  |  24m 34s |  |  branch has no errors when building and testing our client artifacts.  |
   |||| _ Patch Compile Tests _ |
   | +0 :ok: |  mvndep  |   0m 24s |  |  Maven dependency ordering for patch  |
   | +1 :green_heart: |  mvninstall  |   1m  7s |  |  the patch passed  |
   | +1 :green_heart: |  compile  |   9m 56s |  |  the patch passed with JDK Ubuntu-11.0.16+8-post-Ubuntu-0ubuntu120.04  |
   | +1 :green_heart: |  javac  |   9m 56s |  |  the patch passed  |
   | +1 :green_heart: |  compile  |   9m  4s |  |  the patch passed with JDK Private Build-1.8.0_342-8u342-b07-0ubuntu1~20.04-b07  |
   | +1 :green_heart: |  javac  |   9m  4s |  |  the patch passed  |
   | +1 :green_heart: |  blanks  |   0m  0s |  |  The patch has no blanks issues.  |
   | +1 :green_heart: |  checkstyle  |   1m 52s |  |  the patch passed  |
   | +1 :green_heart: |  mvnsite  |   1m 47s |  |  the patch passed  |
   | +1 :green_heart: |  javadoc  |   1m 46s |  |  the patch passed with JDK Ubuntu-11.0.16+8-post-Ubuntu-0ubuntu120.04  |
   | +1 :green_heart: |  javadoc  |   1m 42s |  |  the patch passed with JDK Private Build-1.8.0_342-8u342-b07-0ubuntu1~20.04-b07  |
   | +1 :green_heart: |  spotbugs  |   3m 23s |  |  the patch passed  |
   | +1 :green_heart: |  shadedclient  |  24m  3s |  |  patch has no errors when building and testing our client artifacts.  |
   |||| _ Other Tests _ |
   | +1 :green_heart: |  unit  |   5m  0s |  |  hadoop-yarn-common in the patch passed.  |
   | +1 :green_heart: |  unit  |   5m 46s |  |  hadoop-yarn-server-router in the patch passed.  |
   | +1 :green_heart: |  asflicense  |   1m  1s |  |  The patch does not generate ASF License warnings.  |
   |  |   | 171m 25s |  |  |
   
   
   | Subsystem | Report/Notes |
   |----------:|:-------------|
   | Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-5030/6/artifact/out/Dockerfile |
   | GITHUB PR | https://github.com/apache/hadoop/pull/5030 |
   | Optional Tests | dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient spotbugs checkstyle codespell detsecrets |
   | uname | Linux 662fb61950d1 4.15.0-191-generic #202-Ubuntu SMP Thu Aug 4 01:49:29 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux |
   | Build tool | maven |
   | Personality | dev-support/bin/hadoop.sh |
   | git revision | trunk / 23dc44045a83303bd1a6e02a8ef9f994b5f48873 |
   | Default Java | Private Build-1.8.0_342-8u342-b07-0ubuntu1~20.04-b07 |
   | Multi-JDK versions | /usr/lib/jvm/java-11-openjdk-amd64:Ubuntu-11.0.16+8-post-Ubuntu-0ubuntu120.04 /usr/lib/jvm/java-8-openjdk-amd64:Private Build-1.8.0_342-8u342-b07-0ubuntu1~20.04-b07 |
   |  Test Results | https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-5030/6/testReport/ |
   | Max. process+thread count | 748 (vs. ulimit of 5500) |
   | modules | C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-router U: hadoop-yarn-project/hadoop-yarn |
   | Console output | https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-5030/6/console |
   | versions | git=2.25.1 maven=3.6.3 spotbugs=4.2.2 |
   | Powered by | Apache Yetus 0.14.0 https://yetus.apache.org |
   
   
   This message was automatically generated.
   
   


-- 
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: common-issues-unsubscribe@hadoop.apache.org

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


[GitHub] [hadoop] slfan1989 commented on a diff in pull request #5030: YARN-11345. [Federation] Refactoring Yarn Router's Application Web Page.

Posted by GitBox <gi...@apache.org>.
slfan1989 commented on code in PR #5030:
URL: https://github.com/apache/hadoop/pull/5030#discussion_r1001179187


##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-router/src/main/java/org/apache/hadoop/yarn/server/router/webapp/AppsBlock.java:
##########
@@ -81,53 +155,57 @@ protected void render(Block html) {
 
     // Render the applications
     StringBuilder appsTableData = new StringBuilder("[\n");
-    for (AppInfo app : apps.getApps()) {
-      try {
-
-        String percent = String.format("%.1f", app.getProgress() * 100.0F);
-        String trackingURL =
-            app.getTrackingUrl() == null ? "#" : app.getTrackingUrl();
-        // AppID numerical value parsed by parseHadoopID in yarn.dt.plugins.js
-        appsTableData.append("[\"")
-            .append("<a href='").append(trackingURL).append("'>")
-            .append(app.getAppId()).append("</a>\",\"")
-            .append(escape(app.getUser())).append("\",\"")
-            .append(escape(app.getName())).append("\",\"")
-            .append(escape(app.getApplicationType())).append("\",\"")
-            .append(escape(app.getQueue())).append("\",\"")
-            .append(String.valueOf(app.getPriority())).append("\",\"")
-            .append(app.getStartTime()).append("\",\"")
-            .append(app.getFinishTime()).append("\",\"")
-            .append(app.getState()).append("\",\"")
-            .append(app.getFinalStatus()).append("\",\"")
-            // Progress bar
-            .append("<br title='").append(percent).append("'> <div class='")
-            .append(C_PROGRESSBAR).append("' title='")
-            .append(join(percent, '%')).append("'> ").append("<div class='")
-            .append(C_PROGRESSBAR_VALUE).append("' style='")
-            .append(join("width:", percent, '%')).append("'> </div> </div>")
-            // History link
-            .append("\",\"<a href='").append(trackingURL).append("'>")
-            .append("History").append("</a>");
-        appsTableData.append("\"],\n");
-
-      } catch (Exception e) {
-        LOG.info(
-            "Cannot add application {}: {}", app.getAppId(), e.getMessage());
+
+    if (appsInfo != null) {
+      Collection<AppInfo> apps = appsInfo.getApps();
+      if (CollectionUtils.isNotEmpty(apps)) {
+        int numApps = apps.size();
+        int i = 0;

Review Comment:
   I put i++ after try...catch and the loop can end even if the application has errors.



-- 
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: common-issues-unsubscribe@hadoop.apache.org

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


[GitHub] [hadoop] slfan1989 commented on pull request #5030: YARN-11345. [Federation] Refactoring Yarn Router's Application Web Page.

Posted by GitBox <gi...@apache.org>.
slfan1989 commented on PR #5030:
URL: https://github.com/apache/hadoop/pull/5030#issuecomment-1282523309

   @goiri Can you help review this pr? Thank you very much!


-- 
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: common-issues-unsubscribe@hadoop.apache.org

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


[GitHub] [hadoop] hadoop-yetus commented on pull request #5030: YARN-11345. [Federation] Refactoring Yarn Router's Application Web Page.

Posted by GitBox <gi...@apache.org>.
hadoop-yetus commented on PR #5030:
URL: https://github.com/apache/hadoop/pull/5030#issuecomment-1278694125

   :confetti_ball: **+1 overall**
   
   
   
   
   
   
   | Vote | Subsystem | Runtime |  Logfile | Comment |
   |:----:|----------:|--------:|:--------:|:-------:|
   | +0 :ok: |  reexec  |   0m 57s |  |  Docker mode activated.  |
   |||| _ Prechecks _ |
   | +1 :green_heart: |  dupname  |   0m  0s |  |  No case conflicting files found.  |
   | +0 :ok: |  codespell  |   0m  1s |  |  codespell was not available.  |
   | +0 :ok: |  detsecrets  |   0m  1s |  |  detect-secrets was not available.  |
   | +1 :green_heart: |  @author  |   0m  0s |  |  The patch does not contain any @author tags.  |
   | +1 :green_heart: |  test4tests  |   0m  0s |  |  The patch appears to include 1 new or modified test files.  |
   |||| _ trunk Compile Tests _ |
   | +0 :ok: |  mvndep  |  15m 21s |  |  Maven dependency ordering for branch  |
   | +1 :green_heart: |  mvninstall  |  28m 54s |  |  trunk passed  |
   | +1 :green_heart: |  compile  |  10m 46s |  |  trunk passed with JDK Ubuntu-11.0.16+8-post-Ubuntu-0ubuntu120.04  |
   | +1 :green_heart: |  compile  |   9m 10s |  |  trunk passed with JDK Private Build-1.8.0_342-8u342-b07-0ubuntu1~20.04-b07  |
   | +1 :green_heart: |  checkstyle  |   2m  4s |  |  trunk passed  |
   | +1 :green_heart: |  mvnsite  |   2m  3s |  |  trunk passed  |
   | +1 :green_heart: |  javadoc  |   2m  3s |  |  trunk passed with JDK Ubuntu-11.0.16+8-post-Ubuntu-0ubuntu120.04  |
   | +1 :green_heart: |  javadoc  |   1m 54s |  |  trunk passed with JDK Private Build-1.8.0_342-8u342-b07-0ubuntu1~20.04-b07  |
   | +1 :green_heart: |  spotbugs  |   3m 27s |  |  trunk passed  |
   | +1 :green_heart: |  shadedclient  |  24m 35s |  |  branch has no errors when building and testing our client artifacts.  |
   |||| _ Patch Compile Tests _ |
   | +0 :ok: |  mvndep  |   0m 25s |  |  Maven dependency ordering for patch  |
   | +1 :green_heart: |  mvninstall  |   1m  8s |  |  the patch passed  |
   | +1 :green_heart: |  compile  |   9m 56s |  |  the patch passed with JDK Ubuntu-11.0.16+8-post-Ubuntu-0ubuntu120.04  |
   | +1 :green_heart: |  javac  |   9m 56s |  |  the patch passed  |
   | +1 :green_heart: |  compile  |   9m  1s |  |  the patch passed with JDK Private Build-1.8.0_342-8u342-b07-0ubuntu1~20.04-b07  |
   | +1 :green_heart: |  javac  |   9m  1s |  |  the patch passed  |
   | +1 :green_heart: |  blanks  |   0m  0s |  |  The patch has no blanks issues.  |
   | +1 :green_heart: |  checkstyle  |   1m 50s |  |  the patch passed  |
   | +1 :green_heart: |  mvnsite  |   1m 50s |  |  the patch passed  |
   | +1 :green_heart: |  javadoc  |   1m 45s |  |  the patch passed with JDK Ubuntu-11.0.16+8-post-Ubuntu-0ubuntu120.04  |
   | +1 :green_heart: |  javadoc  |   1m 41s |  |  the patch passed with JDK Private Build-1.8.0_342-8u342-b07-0ubuntu1~20.04-b07  |
   | +1 :green_heart: |  spotbugs  |   3m 23s |  |  the patch passed  |
   | +1 :green_heart: |  shadedclient  |  24m 12s |  |  patch has no errors when building and testing our client artifacts.  |
   |||| _ Other Tests _ |
   | +1 :green_heart: |  unit  |   4m 59s |  |  hadoop-yarn-common in the patch passed.  |
   | +1 :green_heart: |  unit  |   5m 31s |  |  hadoop-yarn-server-router in the patch passed.  |
   | +1 :green_heart: |  asflicense  |   1m  1s |  |  The patch does not generate ASF License warnings.  |
   |  |   | 171m 38s |  |  |
   
   
   | Subsystem | Report/Notes |
   |----------:|:-------------|
   | Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-5030/1/artifact/out/Dockerfile |
   | GITHUB PR | https://github.com/apache/hadoop/pull/5030 |
   | Optional Tests | dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient spotbugs checkstyle codespell detsecrets |
   | uname | Linux 2afe1e6dde46 4.15.0-191-generic #202-Ubuntu SMP Thu Aug 4 01:49:29 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux |
   | Build tool | maven |
   | Personality | dev-support/bin/hadoop.sh |
   | git revision | trunk / cda98cd901cb9c8bbda05a631461ab1886c454b7 |
   | Default Java | Private Build-1.8.0_342-8u342-b07-0ubuntu1~20.04-b07 |
   | Multi-JDK versions | /usr/lib/jvm/java-11-openjdk-amd64:Ubuntu-11.0.16+8-post-Ubuntu-0ubuntu120.04 /usr/lib/jvm/java-8-openjdk-amd64:Private Build-1.8.0_342-8u342-b07-0ubuntu1~20.04-b07 |
   |  Test Results | https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-5030/1/testReport/ |
   | Max. process+thread count | 713 (vs. ulimit of 5500) |
   | modules | C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-router U: hadoop-yarn-project/hadoop-yarn |
   | Console output | https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-5030/1/console |
   | versions | git=2.25.1 maven=3.6.3 spotbugs=4.2.2 |
   | Powered by | Apache Yetus 0.14.0 https://yetus.apache.org |
   
   
   This message was automatically generated.
   
   


-- 
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: common-issues-unsubscribe@hadoop.apache.org

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


[GitHub] [hadoop] hadoop-yetus commented on pull request #5030: YARN-11345. [Federation] Refactoring Yarn Router's Application Web Page.

Posted by GitBox <gi...@apache.org>.
hadoop-yetus commented on PR #5030:
URL: https://github.com/apache/hadoop/pull/5030#issuecomment-1280479090

   :confetti_ball: **+1 overall**
   
   
   
   
   
   
   | Vote | Subsystem | Runtime |  Logfile | Comment |
   |:----:|----------:|--------:|:--------:|:-------:|
   | +0 :ok: |  reexec  |   1m  0s |  |  Docker mode activated.  |
   |||| _ Prechecks _ |
   | +1 :green_heart: |  dupname  |   0m  0s |  |  No case conflicting files found.  |
   | +0 :ok: |  codespell  |   0m  1s |  |  codespell was not available.  |
   | +0 :ok: |  detsecrets  |   0m  1s |  |  detect-secrets was not available.  |
   | +1 :green_heart: |  @author  |   0m  0s |  |  The patch does not contain any @author tags.  |
   | +1 :green_heart: |  test4tests  |   0m  0s |  |  The patch appears to include 1 new or modified test files.  |
   |||| _ trunk Compile Tests _ |
   | +0 :ok: |  mvndep  |  15m 23s |  |  Maven dependency ordering for branch  |
   | +1 :green_heart: |  mvninstall  |  28m 39s |  |  trunk passed  |
   | +1 :green_heart: |  compile  |  10m 39s |  |  trunk passed with JDK Ubuntu-11.0.16+8-post-Ubuntu-0ubuntu120.04  |
   | +1 :green_heart: |  compile  |   9m 12s |  |  trunk passed with JDK Private Build-1.8.0_342-8u342-b07-0ubuntu1~20.04-b07  |
   | +1 :green_heart: |  checkstyle  |   2m  4s |  |  trunk passed  |
   | +1 :green_heart: |  mvnsite  |   2m  2s |  |  trunk passed  |
   | +1 :green_heart: |  javadoc  |   2m  5s |  |  trunk passed with JDK Ubuntu-11.0.16+8-post-Ubuntu-0ubuntu120.04  |
   | +1 :green_heart: |  javadoc  |   1m 53s |  |  trunk passed with JDK Private Build-1.8.0_342-8u342-b07-0ubuntu1~20.04-b07  |
   | +1 :green_heart: |  spotbugs  |   3m 22s |  |  trunk passed  |
   | +1 :green_heart: |  shadedclient  |  24m 14s |  |  branch has no errors when building and testing our client artifacts.  |
   |||| _ Patch Compile Tests _ |
   | +0 :ok: |  mvndep  |   0m 25s |  |  Maven dependency ordering for patch  |
   | +1 :green_heart: |  mvninstall  |   1m  7s |  |  the patch passed  |
   | +1 :green_heart: |  compile  |   9m 58s |  |  the patch passed with JDK Ubuntu-11.0.16+8-post-Ubuntu-0ubuntu120.04  |
   | +1 :green_heart: |  javac  |   9m 58s |  |  the patch passed  |
   | +1 :green_heart: |  compile  |   9m  9s |  |  the patch passed with JDK Private Build-1.8.0_342-8u342-b07-0ubuntu1~20.04-b07  |
   | +1 :green_heart: |  javac  |   9m  9s |  |  the patch passed  |
   | +1 :green_heart: |  blanks  |   0m  0s |  |  The patch has no blanks issues.  |
   | +1 :green_heart: |  checkstyle  |   1m 52s |  |  the patch passed  |
   | +1 :green_heart: |  mvnsite  |   1m 47s |  |  the patch passed  |
   | +1 :green_heart: |  javadoc  |   1m 43s |  |  the patch passed with JDK Ubuntu-11.0.16+8-post-Ubuntu-0ubuntu120.04  |
   | +1 :green_heart: |  javadoc  |   1m 40s |  |  the patch passed with JDK Private Build-1.8.0_342-8u342-b07-0ubuntu1~20.04-b07  |
   | +1 :green_heart: |  spotbugs  |   3m 26s |  |  the patch passed  |
   | +1 :green_heart: |  shadedclient  |  24m 29s |  |  patch has no errors when building and testing our client artifacts.  |
   |||| _ Other Tests _ |
   | +1 :green_heart: |  unit  |   5m  2s |  |  hadoop-yarn-common in the patch passed.  |
   | +1 :green_heart: |  unit  |   5m 30s |  |  hadoop-yarn-server-router in the patch passed.  |
   | +1 :green_heart: |  asflicense  |   1m  4s |  |  The patch does not generate ASF License warnings.  |
   |  |   | 171m 19s |  |  |
   
   
   | Subsystem | Report/Notes |
   |----------:|:-------------|
   | Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-5030/2/artifact/out/Dockerfile |
   | GITHUB PR | https://github.com/apache/hadoop/pull/5030 |
   | Optional Tests | dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient spotbugs checkstyle codespell detsecrets |
   | uname | Linux f744203f78ba 4.15.0-191-generic #202-Ubuntu SMP Thu Aug 4 01:49:29 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux |
   | Build tool | maven |
   | Personality | dev-support/bin/hadoop.sh |
   | git revision | trunk / bd620bf211c24da34bb732fd2d5f6c1e1031760b |
   | Default Java | Private Build-1.8.0_342-8u342-b07-0ubuntu1~20.04-b07 |
   | Multi-JDK versions | /usr/lib/jvm/java-11-openjdk-amd64:Ubuntu-11.0.16+8-post-Ubuntu-0ubuntu120.04 /usr/lib/jvm/java-8-openjdk-amd64:Private Build-1.8.0_342-8u342-b07-0ubuntu1~20.04-b07 |
   |  Test Results | https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-5030/2/testReport/ |
   | Max. process+thread count | 711 (vs. ulimit of 5500) |
   | modules | C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-router U: hadoop-yarn-project/hadoop-yarn |
   | Console output | https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-5030/2/console |
   | versions | git=2.25.1 maven=3.6.3 spotbugs=4.2.2 |
   | Powered by | Apache Yetus 0.14.0 https://yetus.apache.org |
   
   
   This message was automatically generated.
   
   


-- 
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: common-issues-unsubscribe@hadoop.apache.org

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


[GitHub] [hadoop] slfan1989 commented on pull request #5030: YARN-11345. [Federation] Refactoring Yarn Router's Application Web Page.

Posted by GitBox <gi...@apache.org>.
slfan1989 commented on PR #5030:
URL: https://github.com/apache/hadoop/pull/5030#issuecomment-1289132011

   @goiri Can you help to merge this pr into trunk branch? Thank you very much! I will follow up with YARN-11354 [Federation] Add Yarn Router's NodeLabel Web Page.


-- 
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: common-issues-unsubscribe@hadoop.apache.org

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


[GitHub] [hadoop] goiri merged pull request #5030: YARN-11345. [Federation] Refactoring Yarn Router's Application Web Page.

Posted by GitBox <gi...@apache.org>.
goiri merged PR #5030:
URL: https://github.com/apache/hadoop/pull/5030


-- 
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: common-issues-unsubscribe@hadoop.apache.org

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


[GitHub] [hadoop] slfan1989 commented on a diff in pull request #5030: YARN-11345. [Federation] Refactoring Yarn Router's Application Web Page.

Posted by GitBox <gi...@apache.org>.
slfan1989 commented on code in PR #5030:
URL: https://github.com/apache/hadoop/pull/5030#discussion_r1000883075


##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-router/src/main/java/org/apache/hadoop/yarn/server/router/webapp/AppsBlock.java:
##########
@@ -81,53 +154,57 @@ protected void render(Block html) {
 
     // Render the applications
     StringBuilder appsTableData = new StringBuilder("[\n");
-    for (AppInfo app : apps.getApps()) {
-      try {
-
-        String percent = String.format("%.1f", app.getProgress() * 100.0F);
-        String trackingURL =
-            app.getTrackingUrl() == null ? "#" : app.getTrackingUrl();
-        // AppID numerical value parsed by parseHadoopID in yarn.dt.plugins.js
-        appsTableData.append("[\"")
-            .append("<a href='").append(trackingURL).append("'>")
-            .append(app.getAppId()).append("</a>\",\"")
-            .append(escape(app.getUser())).append("\",\"")
-            .append(escape(app.getName())).append("\",\"")
-            .append(escape(app.getApplicationType())).append("\",\"")
-            .append(escape(app.getQueue())).append("\",\"")
-            .append(String.valueOf(app.getPriority())).append("\",\"")
-            .append(app.getStartTime()).append("\",\"")
-            .append(app.getFinishTime()).append("\",\"")
-            .append(app.getState()).append("\",\"")
-            .append(app.getFinalStatus()).append("\",\"")
-            // Progress bar
-            .append("<br title='").append(percent).append("'> <div class='")
-            .append(C_PROGRESSBAR).append("' title='")
-            .append(join(percent, '%')).append("'> ").append("<div class='")
-            .append(C_PROGRESSBAR_VALUE).append("' style='")
-            .append(join("width:", percent, '%')).append("'> </div> </div>")
-            // History link
-            .append("\",\"<a href='").append(trackingURL).append("'>")
-            .append("History").append("</a>");
-        appsTableData.append("\"],\n");
-
-      } catch (Exception e) {
-        LOG.info(
-            "Cannot add application {}: {}", app.getAppId(), e.getMessage());
+
+    if (appsInfo != null && CollectionUtils.isNotEmpty(appsInfo.getApps())) {
+      for (AppInfo app : appsInfo.getApps()) {
+        try {
+
+          String percent = String.format("%.1f", app.getProgress() * 100.0F);
+          String trackingURL =
+              app.getTrackingUrl() == null ? "#" : app.getTrackingUrl();
+
+          // AppID numerical value parsed by parseHadoopID in yarn.dt.plugins.js
+          appsTableData.append("[\"")
+              .append("<a href='").append(trackingURL).append("'>")
+              .append(app.getAppId()).append("</a>\",\"")
+              .append(escape(app.getUser())).append("\",\"")
+              .append(escape(app.getName())).append("\",\"")
+              .append(escape(app.getApplicationType())).append("\",\"")
+              .append(escape(app.getQueue())).append("\",\"")
+              .append(app.getPriority()).append("\",\"")
+              .append(app.getStartTime()).append("\",\"")
+              .append(app.getFinishTime()).append("\",\"")
+              .append(app.getState()).append("\",\"")
+              .append(app.getFinalStatus()).append("\",\"")
+              // Progress bar
+              .append("<br title='").append(percent).append("'> <div class='")
+              .append(C_PROGRESSBAR).append("' title='")
+              .append(join(percent, '%')).append("'> ").append("<div class='")
+              .append(C_PROGRESSBAR_VALUE).append("' style='")
+              .append(join("width:", percent, '%')).append("'> </div> </div>")
+              // History link
+              .append("\",\"<a href='").append(trackingURL).append("'>")
+              .append("History").append("</a>");
+          appsTableData.append("\"],\n");
+
+        } catch (Exception e) {
+          LOG.info("Cannot add application {}: {}", app.getAppId(), e.getMessage());
+        }
+      }
+
+      // The purpose of this part of the code is to remove redundant commas.

Review Comment:
   Thanks for your suggestion, I will refactor this part of the code.



-- 
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: common-issues-unsubscribe@hadoop.apache.org

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


[GitHub] [hadoop] slfan1989 commented on pull request #5030: YARN-11345. [Federation] Refactoring Yarn Router's Application Web Page.

Posted by GitBox <gi...@apache.org>.
slfan1989 commented on PR #5030:
URL: https://github.com/apache/hadoop/pull/5030#issuecomment-1289473469

   @goiri Thank you very much for helping to review the code!


-- 
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: common-issues-unsubscribe@hadoop.apache.org

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


[GitHub] [hadoop] slfan1989 commented on a diff in pull request #5030: YARN-11345. [Federation] Refactoring Yarn Router's Application Web Page.

Posted by GitBox <gi...@apache.org>.
slfan1989 commented on code in PR #5030:
URL: https://github.com/apache/hadoop/pull/5030#discussion_r1001269099


##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-router/src/main/java/org/apache/hadoop/yarn/server/router/webapp/AppsBlock.java:
##########
@@ -81,53 +155,57 @@ protected void render(Block html) {
 
     // Render the applications
     StringBuilder appsTableData = new StringBuilder("[\n");
-    for (AppInfo app : apps.getApps()) {
-      try {
-
-        String percent = String.format("%.1f", app.getProgress() * 100.0F);
-        String trackingURL =
-            app.getTrackingUrl() == null ? "#" : app.getTrackingUrl();
-        // AppID numerical value parsed by parseHadoopID in yarn.dt.plugins.js
-        appsTableData.append("[\"")
-            .append("<a href='").append(trackingURL).append("'>")
-            .append(app.getAppId()).append("</a>\",\"")
-            .append(escape(app.getUser())).append("\",\"")
-            .append(escape(app.getName())).append("\",\"")
-            .append(escape(app.getApplicationType())).append("\",\"")
-            .append(escape(app.getQueue())).append("\",\"")
-            .append(String.valueOf(app.getPriority())).append("\",\"")
-            .append(app.getStartTime()).append("\",\"")
-            .append(app.getFinishTime()).append("\",\"")
-            .append(app.getState()).append("\",\"")
-            .append(app.getFinalStatus()).append("\",\"")
-            // Progress bar
-            .append("<br title='").append(percent).append("'> <div class='")
-            .append(C_PROGRESSBAR).append("' title='")
-            .append(join(percent, '%')).append("'> ").append("<div class='")
-            .append(C_PROGRESSBAR_VALUE).append("' style='")
-            .append(join("width:", percent, '%')).append("'> </div> </div>")
-            // History link
-            .append("\",\"<a href='").append(trackingURL).append("'>")
-            .append("History").append("</a>");
-        appsTableData.append("\"],\n");
-
-      } catch (Exception e) {
-        LOG.info(
-            "Cannot add application {}: {}", app.getAppId(), e.getMessage());
+
+    if (appsInfo != null) {
+      Collection<AppInfo> apps = appsInfo.getApps();
+      if (CollectionUtils.isNotEmpty(apps)) {
+        int numApps = apps.size();
+        int i = 0;
+        for (AppInfo app : apps) {
+          try {
+            String percent = String.format("%.1f", app.getProgress() * 100.0F);
+            String trackingURL =
+                app.getTrackingUrl() == null ? "#" : app.getTrackingUrl();
+
+            // AppID numerical value parsed by parseHadoopID in yarn.dt.plugins.js
+            appsTableData.append("[\"")
+                .append("<a href='").append(trackingURL).append("'>")
+                .append(app.getAppId()).append("</a>\",\"")
+                .append(escape(app.getUser())).append("\",\"")
+                .append(escape(app.getName())).append("\",\"")
+                .append(escape(app.getApplicationType())).append("\",\"")
+                .append(escape(app.getQueue())).append("\",\"")
+                .append(app.getPriority()).append("\",\"")
+                .append(app.getStartTime()).append("\",\"")
+                .append(app.getFinishTime()).append("\",\"")
+                .append(app.getState()).append("\",\"")
+                .append(app.getFinalStatus()).append("\",\"")
+                // Progress bar
+                .append("<br title='").append(percent).append("'> <div class='")
+                .append(C_PROGRESSBAR).append("' title='")
+                .append(join(percent, '%')).append("'> ").append("<div class='")
+                .append(C_PROGRESSBAR_VALUE).append("' style='")
+                .append(join("width:", percent, '%')).append("'> </div> </div>")
+                // History link
+                .append("\",\"<a href='").append(trackingURL).append("'>")
+                .append("History").append("</a>");
+            appsTableData.append("\"]\n");
+
+            if (i < numApps - 1) {
+              appsTableData.append(",");
+            }
+          } catch (Exception e) {
+            LOG.info("Cannot add application {}: {}", app.getAppId(), e.getMessage());
+          }
+          i++;

Review Comment:
   I read this part of the code carefully, the original code is reasonable, we should remove the extra comma outside the loop because we can't tell where the last comma is
   
   For example:
   We have 4 apps, A, B, C, D, and we expect the result to be [A, B, C, D], but when traversing the app list, the last app D fails, and we end up with [A ,B,C,], the last app D fails, we can't handle this situation inside the loop
   
   But the readability of the original logic is not good, I refactored this part of the code.



-- 
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: common-issues-unsubscribe@hadoop.apache.org

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


[GitHub] [hadoop] hadoop-yetus commented on pull request #5030: YARN-11345. [Federation] Refactoring Yarn Router's Application Web Page.

Posted by GitBox <gi...@apache.org>.
hadoop-yetus commented on PR #5030:
URL: https://github.com/apache/hadoop/pull/5030#issuecomment-1286438510

   :confetti_ball: **+1 overall**
   
   
   
   
   
   
   | Vote | Subsystem | Runtime |  Logfile | Comment |
   |:----:|----------:|--------:|:--------:|:-------:|
   | +0 :ok: |  reexec  |   0m 41s |  |  Docker mode activated.  |
   |||| _ Prechecks _ |
   | +1 :green_heart: |  dupname  |   0m  0s |  |  No case conflicting files found.  |
   | +0 :ok: |  codespell  |   0m  0s |  |  codespell was not available.  |
   | +0 :ok: |  detsecrets  |   0m  0s |  |  detect-secrets was not available.  |
   | +1 :green_heart: |  @author  |   0m  1s |  |  The patch does not contain any @author tags.  |
   | +1 :green_heart: |  test4tests  |   0m  0s |  |  The patch appears to include 1 new or modified test files.  |
   |||| _ trunk Compile Tests _ |
   | +0 :ok: |  mvndep  |  18m 31s |  |  Maven dependency ordering for branch  |
   | +1 :green_heart: |  mvninstall  |  27m 51s |  |  trunk passed  |
   | +1 :green_heart: |  compile  |  10m 44s |  |  trunk passed with JDK Ubuntu-11.0.16+8-post-Ubuntu-0ubuntu120.04  |
   | +1 :green_heart: |  compile  |   9m 17s |  |  trunk passed with JDK Private Build-1.8.0_342-8u342-b07-0ubuntu1~20.04-b07  |
   | +1 :green_heart: |  checkstyle  |   2m  4s |  |  trunk passed  |
   | +1 :green_heart: |  mvnsite  |   2m 10s |  |  trunk passed  |
   | +1 :green_heart: |  javadoc  |   2m 22s |  |  trunk passed with JDK Ubuntu-11.0.16+8-post-Ubuntu-0ubuntu120.04  |
   | +1 :green_heart: |  javadoc  |   2m 11s |  |  trunk passed with JDK Private Build-1.8.0_342-8u342-b07-0ubuntu1~20.04-b07  |
   | +1 :green_heart: |  spotbugs  |   3m 38s |  |  trunk passed  |
   | +1 :green_heart: |  shadedclient  |  22m 14s |  |  branch has no errors when building and testing our client artifacts.  |
   |||| _ Patch Compile Tests _ |
   | +0 :ok: |  mvndep  |   0m 27s |  |  Maven dependency ordering for patch  |
   | +1 :green_heart: |  mvninstall  |   1m 15s |  |  the patch passed  |
   | +1 :green_heart: |  compile  |   9m 51s |  |  the patch passed with JDK Ubuntu-11.0.16+8-post-Ubuntu-0ubuntu120.04  |
   | +1 :green_heart: |  javac  |   9m 51s |  |  the patch passed  |
   | +1 :green_heart: |  compile  |   9m  4s |  |  the patch passed with JDK Private Build-1.8.0_342-8u342-b07-0ubuntu1~20.04-b07  |
   | +1 :green_heart: |  javac  |   9m  4s |  |  the patch passed  |
   | +1 :green_heart: |  blanks  |   0m  0s |  |  The patch has no blanks issues.  |
   | -0 :warning: |  checkstyle  |   1m 47s | [/results-checkstyle-hadoop-yarn-project_hadoop-yarn.txt](https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-5030/5/artifact/out/results-checkstyle-hadoop-yarn-project_hadoop-yarn.txt) |  hadoop-yarn-project/hadoop-yarn: The patch generated 1 new + 3 unchanged - 0 fixed = 4 total (was 3)  |
   | +1 :green_heart: |  mvnsite  |   1m 59s |  |  the patch passed  |
   | +1 :green_heart: |  javadoc  |   1m 59s |  |  the patch passed with JDK Ubuntu-11.0.16+8-post-Ubuntu-0ubuntu120.04  |
   | +1 :green_heart: |  javadoc  |   1m 53s |  |  the patch passed with JDK Private Build-1.8.0_342-8u342-b07-0ubuntu1~20.04-b07  |
   | +1 :green_heart: |  spotbugs  |   3m 35s |  |  the patch passed  |
   | +1 :green_heart: |  shadedclient  |  24m 31s |  |  patch has no errors when building and testing our client artifacts.  |
   |||| _ Other Tests _ |
   | +1 :green_heart: |  unit  |   5m 21s |  |  hadoop-yarn-common in the patch passed.  |
   | +1 :green_heart: |  unit  |   5m 49s |  |  hadoop-yarn-server-router in the patch passed.  |
   | +1 :green_heart: |  asflicense  |   1m 14s |  |  The patch does not generate ASF License warnings.  |
   |  |   | 174m 24s |  |  |
   
   
   | Subsystem | Report/Notes |
   |----------:|:-------------|
   | Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-5030/5/artifact/out/Dockerfile |
   | GITHUB PR | https://github.com/apache/hadoop/pull/5030 |
   | Optional Tests | dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient spotbugs checkstyle codespell detsecrets |
   | uname | Linux 89d75cd3feb7 4.15.0-191-generic #202-Ubuntu SMP Thu Aug 4 01:49:29 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux |
   | Build tool | maven |
   | Personality | dev-support/bin/hadoop.sh |
   | git revision | trunk / d2b820682398e8f8f449b089a7227c99c26e1c1e |
   | Default Java | Private Build-1.8.0_342-8u342-b07-0ubuntu1~20.04-b07 |
   | Multi-JDK versions | /usr/lib/jvm/java-11-openjdk-amd64:Ubuntu-11.0.16+8-post-Ubuntu-0ubuntu120.04 /usr/lib/jvm/java-8-openjdk-amd64:Private Build-1.8.0_342-8u342-b07-0ubuntu1~20.04-b07 |
   |  Test Results | https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-5030/5/testReport/ |
   | Max. process+thread count | 814 (vs. ulimit of 5500) |
   | modules | C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-router U: hadoop-yarn-project/hadoop-yarn |
   | Console output | https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-5030/5/console |
   | versions | git=2.25.1 maven=3.6.3 spotbugs=4.2.2 |
   | Powered by | Apache Yetus 0.14.0 https://yetus.apache.org |
   
   
   This message was automatically generated.
   
   


-- 
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: common-issues-unsubscribe@hadoop.apache.org

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


[GitHub] [hadoop] slfan1989 commented on a diff in pull request #5030: YARN-11345. [Federation] Refactoring Yarn Router's Application Web Page.

Posted by GitBox <gi...@apache.org>.
slfan1989 commented on code in PR #5030:
URL: https://github.com/apache/hadoop/pull/5030#discussion_r999929531


##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-router/src/main/java/org/apache/hadoop/yarn/server/router/webapp/AppsBlock.java:
##########
@@ -81,53 +154,53 @@ protected void render(Block html) {
 
     // Render the applications
     StringBuilder appsTableData = new StringBuilder("[\n");
-    for (AppInfo app : apps.getApps()) {
-      try {
-
-        String percent = String.format("%.1f", app.getProgress() * 100.0F);
-        String trackingURL =
-            app.getTrackingUrl() == null ? "#" : app.getTrackingUrl();
-        // AppID numerical value parsed by parseHadoopID in yarn.dt.plugins.js
-        appsTableData.append("[\"")
-            .append("<a href='").append(trackingURL).append("'>")
-            .append(app.getAppId()).append("</a>\",\"")
-            .append(escape(app.getUser())).append("\",\"")
-            .append(escape(app.getName())).append("\",\"")
-            .append(escape(app.getApplicationType())).append("\",\"")
-            .append(escape(app.getQueue())).append("\",\"")
-            .append(String.valueOf(app.getPriority())).append("\",\"")
-            .append(app.getStartTime()).append("\",\"")
-            .append(app.getFinishTime()).append("\",\"")
-            .append(app.getState()).append("\",\"")
-            .append(app.getFinalStatus()).append("\",\"")
-            // Progress bar
-            .append("<br title='").append(percent).append("'> <div class='")
-            .append(C_PROGRESSBAR).append("' title='")
-            .append(join(percent, '%')).append("'> ").append("<div class='")
-            .append(C_PROGRESSBAR_VALUE).append("' style='")
-            .append(join("width:", percent, '%')).append("'> </div> </div>")
-            // History link
-            .append("\",\"<a href='").append(trackingURL).append("'>")
-            .append("History").append("</a>");
-        appsTableData.append("\"],\n");
-
-      } catch (Exception e) {
-        LOG.info(
-            "Cannot add application {}: {}", app.getAppId(), e.getMessage());
+
+    if (appsInfo != null && CollectionUtils.isNotEmpty(appsInfo.getApps())) {
+      for (AppInfo app : appsInfo.getApps()) {
+        try {
+
+          String percent = String.format("%.1f", app.getProgress() * 100.0F);
+          String trackingURL =
+              app.getTrackingUrl() == null ? "#" : app.getTrackingUrl();
+
+          // AppID numerical value parsed by parseHadoopID in yarn.dt.plugins.js
+          appsTableData.append("[\"")
+              .append("<a href='").append(trackingURL).append("'>")
+              .append(app.getAppId()).append("</a>\",\"")
+              .append(escape(app.getUser())).append("\",\"")
+              .append(escape(app.getName())).append("\",\"")
+              .append(escape(app.getApplicationType())).append("\",\"")
+              .append(escape(app.getQueue())).append("\",\"")
+              .append(app.getPriority()).append("\",\"")
+              .append(app.getStartTime()).append("\",\"")
+              .append(app.getFinishTime()).append("\",\"")
+              .append(app.getState()).append("\",\"")
+              .append(app.getFinalStatus()).append("\",\"")
+              // Progress bar
+              .append("<br title='").append(percent).append("'> <div class='")
+              .append(C_PROGRESSBAR).append("' title='")
+              .append(join(percent, '%')).append("'> ").append("<div class='")
+              .append(C_PROGRESSBAR_VALUE).append("' style='")
+              .append(join("width:", percent, '%')).append("'> </div> </div>")
+              // History link
+              .append("\",\"<a href='").append(trackingURL).append("'>")
+              .append("History").append("</a>");
+          appsTableData.append("\"],\n");
+
+        } catch (Exception e) {
+          LOG.info(

Review Comment:
   I will fix it.



-- 
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: common-issues-unsubscribe@hadoop.apache.org

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


[GitHub] [hadoop] hadoop-yetus commented on pull request #5030: YARN-11345. [Federation] Refactoring Yarn Router's Application Web Page.

Posted by GitBox <gi...@apache.org>.
hadoop-yetus commented on PR #5030:
URL: https://github.com/apache/hadoop/pull/5030#issuecomment-1284906771

   :confetti_ball: **+1 overall**
   
   
   
   
   
   
   | Vote | Subsystem | Runtime |  Logfile | Comment |
   |:----:|----------:|--------:|:--------:|:-------:|
   | +0 :ok: |  reexec  |   0m 59s |  |  Docker mode activated.  |
   |||| _ Prechecks _ |
   | +1 :green_heart: |  dupname  |   0m  0s |  |  No case conflicting files found.  |
   | +0 :ok: |  codespell  |   0m  0s |  |  codespell was not available.  |
   | +0 :ok: |  detsecrets  |   0m  0s |  |  detect-secrets was not available.  |
   | +1 :green_heart: |  @author  |   0m  0s |  |  The patch does not contain any @author tags.  |
   | +1 :green_heart: |  test4tests  |   0m  0s |  |  The patch appears to include 1 new or modified test files.  |
   |||| _ trunk Compile Tests _ |
   | +0 :ok: |  mvndep  |  17m 44s |  |  Maven dependency ordering for branch  |
   | +1 :green_heart: |  mvninstall  |  29m  3s |  |  trunk passed  |
   | +1 :green_heart: |  compile  |  10m 31s |  |  trunk passed with JDK Ubuntu-11.0.16+8-post-Ubuntu-0ubuntu120.04  |
   | +1 :green_heart: |  compile  |   8m 46s |  |  trunk passed with JDK Private Build-1.8.0_342-8u342-b07-0ubuntu1~20.04-b07  |
   | +1 :green_heart: |  checkstyle  |   1m 44s |  |  trunk passed  |
   | +1 :green_heart: |  mvnsite  |   1m 33s |  |  trunk passed  |
   | +1 :green_heart: |  javadoc  |   1m 34s |  |  trunk passed with JDK Ubuntu-11.0.16+8-post-Ubuntu-0ubuntu120.04  |
   | +1 :green_heart: |  javadoc  |   1m 23s |  |  trunk passed with JDK Private Build-1.8.0_342-8u342-b07-0ubuntu1~20.04-b07  |
   | +1 :green_heart: |  spotbugs  |   2m 54s |  |  trunk passed  |
   | +1 :green_heart: |  shadedclient  |  23m  3s |  |  branch has no errors when building and testing our client artifacts.  |
   |||| _ Patch Compile Tests _ |
   | +0 :ok: |  mvndep  |   0m 24s |  |  Maven dependency ordering for patch  |
   | +1 :green_heart: |  mvninstall  |   1m  0s |  |  the patch passed  |
   | +1 :green_heart: |  compile  |   9m 41s |  |  the patch passed with JDK Ubuntu-11.0.16+8-post-Ubuntu-0ubuntu120.04  |
   | +1 :green_heart: |  javac  |   9m 41s |  |  the patch passed  |
   | +1 :green_heart: |  compile  |   8m 51s |  |  the patch passed with JDK Private Build-1.8.0_342-8u342-b07-0ubuntu1~20.04-b07  |
   | +1 :green_heart: |  javac  |   8m 51s |  |  the patch passed  |
   | +1 :green_heart: |  blanks  |   0m  0s |  |  The patch has no blanks issues.  |
   | +1 :green_heart: |  checkstyle  |   1m 36s |  |  the patch passed  |
   | +1 :green_heart: |  mvnsite  |   1m 24s |  |  the patch passed  |
   | +1 :green_heart: |  javadoc  |   1m 19s |  |  the patch passed with JDK Ubuntu-11.0.16+8-post-Ubuntu-0ubuntu120.04  |
   | +1 :green_heart: |  javadoc  |   1m 15s |  |  the patch passed with JDK Private Build-1.8.0_342-8u342-b07-0ubuntu1~20.04-b07  |
   | +1 :green_heart: |  spotbugs  |   2m 57s |  |  the patch passed  |
   | +1 :green_heart: |  shadedclient  |  23m 35s |  |  patch has no errors when building and testing our client artifacts.  |
   |||| _ Other Tests _ |
   | +1 :green_heart: |  unit  |   4m 45s |  |  hadoop-yarn-common in the patch passed.  |
   | +1 :green_heart: |  unit  |   5m 20s |  |  hadoop-yarn-server-router in the patch passed.  |
   | +1 :green_heart: |  asflicense  |   0m 48s |  |  The patch does not generate ASF License warnings.  |
   |  |   | 165m  0s |  |  |
   
   
   | Subsystem | Report/Notes |
   |----------:|:-------------|
   | Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-5030/3/artifact/out/Dockerfile |
   | GITHUB PR | https://github.com/apache/hadoop/pull/5030 |
   | Optional Tests | dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient spotbugs checkstyle codespell detsecrets |
   | uname | Linux 032c914d0881 4.15.0-192-generic #203-Ubuntu SMP Wed Aug 10 17:40:03 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux |
   | Build tool | maven |
   | Personality | dev-support/bin/hadoop.sh |
   | git revision | trunk / 0cf5fffdbbe962b916b4fa5fd61adcb7f3c817bf |
   | Default Java | Private Build-1.8.0_342-8u342-b07-0ubuntu1~20.04-b07 |
   | Multi-JDK versions | /usr/lib/jvm/java-11-openjdk-amd64:Ubuntu-11.0.16+8-post-Ubuntu-0ubuntu120.04 /usr/lib/jvm/java-8-openjdk-amd64:Private Build-1.8.0_342-8u342-b07-0ubuntu1~20.04-b07 |
   |  Test Results | https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-5030/3/testReport/ |
   | Max. process+thread count | 737 (vs. ulimit of 5500) |
   | modules | C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-router U: hadoop-yarn-project/hadoop-yarn |
   | Console output | https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-5030/3/console |
   | versions | git=2.25.1 maven=3.6.3 spotbugs=4.2.2 |
   | Powered by | Apache Yetus 0.14.0 https://yetus.apache.org |
   
   
   This message was automatically generated.
   
   


-- 
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: common-issues-unsubscribe@hadoop.apache.org

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


[GitHub] [hadoop] slfan1989 commented on a diff in pull request #5030: YARN-11345. [Federation] Refactoring Yarn Router's Application Web Page.

Posted by GitBox <gi...@apache.org>.
slfan1989 commented on code in PR #5030:
URL: https://github.com/apache/hadoop/pull/5030#discussion_r1001179187


##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-router/src/main/java/org/apache/hadoop/yarn/server/router/webapp/AppsBlock.java:
##########
@@ -81,53 +155,57 @@ protected void render(Block html) {
 
     // Render the applications
     StringBuilder appsTableData = new StringBuilder("[\n");
-    for (AppInfo app : apps.getApps()) {
-      try {
-
-        String percent = String.format("%.1f", app.getProgress() * 100.0F);
-        String trackingURL =
-            app.getTrackingUrl() == null ? "#" : app.getTrackingUrl();
-        // AppID numerical value parsed by parseHadoopID in yarn.dt.plugins.js
-        appsTableData.append("[\"")
-            .append("<a href='").append(trackingURL).append("'>")
-            .append(app.getAppId()).append("</a>\",\"")
-            .append(escape(app.getUser())).append("\",\"")
-            .append(escape(app.getName())).append("\",\"")
-            .append(escape(app.getApplicationType())).append("\",\"")
-            .append(escape(app.getQueue())).append("\",\"")
-            .append(String.valueOf(app.getPriority())).append("\",\"")
-            .append(app.getStartTime()).append("\",\"")
-            .append(app.getFinishTime()).append("\",\"")
-            .append(app.getState()).append("\",\"")
-            .append(app.getFinalStatus()).append("\",\"")
-            // Progress bar
-            .append("<br title='").append(percent).append("'> <div class='")
-            .append(C_PROGRESSBAR).append("' title='")
-            .append(join(percent, '%')).append("'> ").append("<div class='")
-            .append(C_PROGRESSBAR_VALUE).append("' style='")
-            .append(join("width:", percent, '%')).append("'> </div> </div>")
-            // History link
-            .append("\",\"<a href='").append(trackingURL).append("'>")
-            .append("History").append("</a>");
-        appsTableData.append("\"],\n");
-
-      } catch (Exception e) {
-        LOG.info(
-            "Cannot add application {}: {}", app.getAppId(), e.getMessage());
+
+    if (appsInfo != null) {
+      Collection<AppInfo> apps = appsInfo.getApps();
+      if (CollectionUtils.isNotEmpty(apps)) {
+        int numApps = apps.size();
+        int i = 0;

Review Comment:
   I put i++ after try...catch, because if any app is affected, the loop is guaranteed to end.



-- 
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: common-issues-unsubscribe@hadoop.apache.org

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


[GitHub] [hadoop] goiri commented on a diff in pull request #5030: YARN-11345. [Federation] Refactoring Yarn Router's Application Web Page.

Posted by GitBox <gi...@apache.org>.
goiri commented on code in PR #5030:
URL: https://github.com/apache/hadoop/pull/5030#discussion_r1000850995


##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-router/src/main/java/org/apache/hadoop/yarn/server/router/webapp/AppsBlock.java:
##########
@@ -81,53 +154,57 @@ protected void render(Block html) {
 
     // Render the applications
     StringBuilder appsTableData = new StringBuilder("[\n");
-    for (AppInfo app : apps.getApps()) {
-      try {
-
-        String percent = String.format("%.1f", app.getProgress() * 100.0F);
-        String trackingURL =
-            app.getTrackingUrl() == null ? "#" : app.getTrackingUrl();
-        // AppID numerical value parsed by parseHadoopID in yarn.dt.plugins.js
-        appsTableData.append("[\"")
-            .append("<a href='").append(trackingURL).append("'>")
-            .append(app.getAppId()).append("</a>\",\"")
-            .append(escape(app.getUser())).append("\",\"")
-            .append(escape(app.getName())).append("\",\"")
-            .append(escape(app.getApplicationType())).append("\",\"")
-            .append(escape(app.getQueue())).append("\",\"")
-            .append(String.valueOf(app.getPriority())).append("\",\"")
-            .append(app.getStartTime()).append("\",\"")
-            .append(app.getFinishTime()).append("\",\"")
-            .append(app.getState()).append("\",\"")
-            .append(app.getFinalStatus()).append("\",\"")
-            // Progress bar
-            .append("<br title='").append(percent).append("'> <div class='")
-            .append(C_PROGRESSBAR).append("' title='")
-            .append(join(percent, '%')).append("'> ").append("<div class='")
-            .append(C_PROGRESSBAR_VALUE).append("' style='")
-            .append(join("width:", percent, '%')).append("'> </div> </div>")
-            // History link
-            .append("\",\"<a href='").append(trackingURL).append("'>")
-            .append("History").append("</a>");
-        appsTableData.append("\"],\n");
-
-      } catch (Exception e) {
-        LOG.info(
-            "Cannot add application {}: {}", app.getAppId(), e.getMessage());
+
+    if (appsInfo != null && CollectionUtils.isNotEmpty(appsInfo.getApps())) {
+      for (AppInfo app : appsInfo.getApps()) {
+        try {
+
+          String percent = String.format("%.1f", app.getProgress() * 100.0F);
+          String trackingURL =
+              app.getTrackingUrl() == null ? "#" : app.getTrackingUrl();
+
+          // AppID numerical value parsed by parseHadoopID in yarn.dt.plugins.js
+          appsTableData.append("[\"")
+              .append("<a href='").append(trackingURL).append("'>")
+              .append(app.getAppId()).append("</a>\",\"")
+              .append(escape(app.getUser())).append("\",\"")
+              .append(escape(app.getName())).append("\",\"")
+              .append(escape(app.getApplicationType())).append("\",\"")
+              .append(escape(app.getQueue())).append("\",\"")
+              .append(app.getPriority()).append("\",\"")
+              .append(app.getStartTime()).append("\",\"")
+              .append(app.getFinishTime()).append("\",\"")
+              .append(app.getState()).append("\",\"")
+              .append(app.getFinalStatus()).append("\",\"")
+              // Progress bar
+              .append("<br title='").append(percent).append("'> <div class='")
+              .append(C_PROGRESSBAR).append("' title='")
+              .append(join(percent, '%')).append("'> ").append("<div class='")
+              .append(C_PROGRESSBAR_VALUE).append("' style='")
+              .append(join("width:", percent, '%')).append("'> </div> </div>")
+              // History link
+              .append("\",\"<a href='").append(trackingURL).append("'>")
+              .append("History").append("</a>");
+          appsTableData.append("\"],\n");
+
+        } catch (Exception e) {
+          LOG.info("Cannot add application {}: {}", app.getAppId(), e.getMessage());
+        }
+      }
+
+      // The purpose of this part of the code is to remove redundant commas.

Review Comment:
   We know how many apps there will be, we should only add it if not the last:
   ```
   if (appsInfo != null) {
     Collection<AppInfo> apps = appsInfo.getApps();
     if (CollectionUtils.isNotEmpty(apps)) {
       int numApps = apps.size();
       for (AppInfo app: apps) {
         ...
         if (i < numApps - 1) {
           appsTableData.append(",");
         }
       }
     }
   }
   ```



-- 
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: common-issues-unsubscribe@hadoop.apache.org

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


[GitHub] [hadoop] slfan1989 commented on a diff in pull request #5030: YARN-11345. [Federation] Refactoring Yarn Router's Application Web Page.

Posted by GitBox <gi...@apache.org>.
slfan1989 commented on code in PR #5030:
URL: https://github.com/apache/hadoop/pull/5030#discussion_r1001214547


##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-router/src/main/java/org/apache/hadoop/yarn/server/router/webapp/AppsBlock.java:
##########
@@ -81,53 +155,57 @@ protected void render(Block html) {
 
     // Render the applications
     StringBuilder appsTableData = new StringBuilder("[\n");
-    for (AppInfo app : apps.getApps()) {
-      try {
-
-        String percent = String.format("%.1f", app.getProgress() * 100.0F);
-        String trackingURL =
-            app.getTrackingUrl() == null ? "#" : app.getTrackingUrl();
-        // AppID numerical value parsed by parseHadoopID in yarn.dt.plugins.js
-        appsTableData.append("[\"")
-            .append("<a href='").append(trackingURL).append("'>")
-            .append(app.getAppId()).append("</a>\",\"")
-            .append(escape(app.getUser())).append("\",\"")
-            .append(escape(app.getName())).append("\",\"")
-            .append(escape(app.getApplicationType())).append("\",\"")
-            .append(escape(app.getQueue())).append("\",\"")
-            .append(String.valueOf(app.getPriority())).append("\",\"")
-            .append(app.getStartTime()).append("\",\"")
-            .append(app.getFinishTime()).append("\",\"")
-            .append(app.getState()).append("\",\"")
-            .append(app.getFinalStatus()).append("\",\"")
-            // Progress bar
-            .append("<br title='").append(percent).append("'> <div class='")
-            .append(C_PROGRESSBAR).append("' title='")
-            .append(join(percent, '%')).append("'> ").append("<div class='")
-            .append(C_PROGRESSBAR_VALUE).append("' style='")
-            .append(join("width:", percent, '%')).append("'> </div> </div>")
-            // History link
-            .append("\",\"<a href='").append(trackingURL).append("'>")
-            .append("History").append("</a>");
-        appsTableData.append("\"],\n");
-
-      } catch (Exception e) {
-        LOG.info(
-            "Cannot add application {}: {}", app.getAppId(), e.getMessage());
+
+    if (appsInfo != null) {
+      Collection<AppInfo> apps = appsInfo.getApps();
+      if (CollectionUtils.isNotEmpty(apps)) {
+        int numApps = apps.size();
+        int i = 0;
+        for (AppInfo app : apps) {
+          try {
+            String percent = String.format("%.1f", app.getProgress() * 100.0F);
+            String trackingURL =
+                app.getTrackingUrl() == null ? "#" : app.getTrackingUrl();
+
+            // AppID numerical value parsed by parseHadoopID in yarn.dt.plugins.js
+            appsTableData.append("[\"")
+                .append("<a href='").append(trackingURL).append("'>")
+                .append(app.getAppId()).append("</a>\",\"")
+                .append(escape(app.getUser())).append("\",\"")
+                .append(escape(app.getName())).append("\",\"")
+                .append(escape(app.getApplicationType())).append("\",\"")
+                .append(escape(app.getQueue())).append("\",\"")
+                .append(app.getPriority()).append("\",\"")
+                .append(app.getStartTime()).append("\",\"")
+                .append(app.getFinishTime()).append("\",\"")
+                .append(app.getState()).append("\",\"")
+                .append(app.getFinalStatus()).append("\",\"")
+                // Progress bar
+                .append("<br title='").append(percent).append("'> <div class='")
+                .append(C_PROGRESSBAR).append("' title='")
+                .append(join(percent, '%')).append("'> ").append("<div class='")
+                .append(C_PROGRESSBAR_VALUE).append("' style='")
+                .append(join("width:", percent, '%')).append("'> </div> </div>")
+                // History link
+                .append("\",\"<a href='").append(trackingURL).append("'>")
+                .append("History").append("</a>");
+            appsTableData.append("\"]\n");
+
+            if (i < numApps - 1) {
+              appsTableData.append(",");
+            }
+          } catch (Exception e) {
+            LOG.info("Cannot add application {}: {}", app.getAppId(), e.getMessage());
+          }
+          i++;

Review Comment:
   Thanks for your suggestion, I will refactor this part of the code to make it more readable.



-- 
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: common-issues-unsubscribe@hadoop.apache.org

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


[GitHub] [hadoop] slfan1989 commented on a diff in pull request #5030: YARN-11345. [Federation] Refactoring Yarn Router's Application Web Page.

Posted by GitBox <gi...@apache.org>.
slfan1989 commented on code in PR #5030:
URL: https://github.com/apache/hadoop/pull/5030#discussion_r999929985


##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-router/src/main/java/org/apache/hadoop/yarn/server/router/webapp/AppsBlock.java:
##########
@@ -81,53 +154,53 @@ protected void render(Block html) {
 
     // Render the applications
     StringBuilder appsTableData = new StringBuilder("[\n");
-    for (AppInfo app : apps.getApps()) {
-      try {
-
-        String percent = String.format("%.1f", app.getProgress() * 100.0F);
-        String trackingURL =
-            app.getTrackingUrl() == null ? "#" : app.getTrackingUrl();
-        // AppID numerical value parsed by parseHadoopID in yarn.dt.plugins.js
-        appsTableData.append("[\"")
-            .append("<a href='").append(trackingURL).append("'>")
-            .append(app.getAppId()).append("</a>\",\"")
-            .append(escape(app.getUser())).append("\",\"")
-            .append(escape(app.getName())).append("\",\"")
-            .append(escape(app.getApplicationType())).append("\",\"")
-            .append(escape(app.getQueue())).append("\",\"")
-            .append(String.valueOf(app.getPriority())).append("\",\"")
-            .append(app.getStartTime()).append("\",\"")
-            .append(app.getFinishTime()).append("\",\"")
-            .append(app.getState()).append("\",\"")
-            .append(app.getFinalStatus()).append("\",\"")
-            // Progress bar
-            .append("<br title='").append(percent).append("'> <div class='")
-            .append(C_PROGRESSBAR).append("' title='")
-            .append(join(percent, '%')).append("'> ").append("<div class='")
-            .append(C_PROGRESSBAR_VALUE).append("' style='")
-            .append(join("width:", percent, '%')).append("'> </div> </div>")
-            // History link
-            .append("\",\"<a href='").append(trackingURL).append("'>")
-            .append("History").append("</a>");
-        appsTableData.append("\"],\n");
-
-      } catch (Exception e) {
-        LOG.info(
-            "Cannot add application {}: {}", app.getAppId(), e.getMessage());
+
+    if (appsInfo != null && CollectionUtils.isNotEmpty(appsInfo.getApps())) {
+      for (AppInfo app : appsInfo.getApps()) {
+        try {
+
+          String percent = String.format("%.1f", app.getProgress() * 100.0F);
+          String trackingURL =
+              app.getTrackingUrl() == null ? "#" : app.getTrackingUrl();
+
+          // AppID numerical value parsed by parseHadoopID in yarn.dt.plugins.js
+          appsTableData.append("[\"")
+              .append("<a href='").append(trackingURL).append("'>")
+              .append(app.getAppId()).append("</a>\",\"")
+              .append(escape(app.getUser())).append("\",\"")
+              .append(escape(app.getName())).append("\",\"")
+              .append(escape(app.getApplicationType())).append("\",\"")
+              .append(escape(app.getQueue())).append("\",\"")
+              .append(app.getPriority()).append("\",\"")
+              .append(app.getStartTime()).append("\",\"")
+              .append(app.getFinishTime()).append("\",\"")
+              .append(app.getState()).append("\",\"")
+              .append(app.getFinalStatus()).append("\",\"")
+              // Progress bar
+              .append("<br title='").append(percent).append("'> <div class='")
+              .append(C_PROGRESSBAR).append("' title='")
+              .append(join(percent, '%')).append("'> ").append("<div class='")
+              .append(C_PROGRESSBAR_VALUE).append("' style='")
+              .append(join("width:", percent, '%')).append("'> </div> </div>")
+              // History link
+              .append("\",\"<a href='").append(trackingURL).append("'>")
+              .append("History").append("</a>");
+          appsTableData.append("\"],\n");
+
+        } catch (Exception e) {
+          LOG.info(
+                  "Cannot add application {}: {}", app.getAppId(), e.getMessage());
+        }
+      }
+      if (appsTableData.charAt(appsTableData.length() - 2) == ',') {

Review Comment:
   I will modify the code.



-- 
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: common-issues-unsubscribe@hadoop.apache.org

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


[GitHub] [hadoop] slfan1989 commented on a diff in pull request #5030: YARN-11345. [Federation] Refactoring Yarn Router's Application Web Page.

Posted by GitBox <gi...@apache.org>.
slfan1989 commented on code in PR #5030:
URL: https://github.com/apache/hadoop/pull/5030#discussion_r1001269099


##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-router/src/main/java/org/apache/hadoop/yarn/server/router/webapp/AppsBlock.java:
##########
@@ -81,53 +155,57 @@ protected void render(Block html) {
 
     // Render the applications
     StringBuilder appsTableData = new StringBuilder("[\n");
-    for (AppInfo app : apps.getApps()) {
-      try {
-
-        String percent = String.format("%.1f", app.getProgress() * 100.0F);
-        String trackingURL =
-            app.getTrackingUrl() == null ? "#" : app.getTrackingUrl();
-        // AppID numerical value parsed by parseHadoopID in yarn.dt.plugins.js
-        appsTableData.append("[\"")
-            .append("<a href='").append(trackingURL).append("'>")
-            .append(app.getAppId()).append("</a>\",\"")
-            .append(escape(app.getUser())).append("\",\"")
-            .append(escape(app.getName())).append("\",\"")
-            .append(escape(app.getApplicationType())).append("\",\"")
-            .append(escape(app.getQueue())).append("\",\"")
-            .append(String.valueOf(app.getPriority())).append("\",\"")
-            .append(app.getStartTime()).append("\",\"")
-            .append(app.getFinishTime()).append("\",\"")
-            .append(app.getState()).append("\",\"")
-            .append(app.getFinalStatus()).append("\",\"")
-            // Progress bar
-            .append("<br title='").append(percent).append("'> <div class='")
-            .append(C_PROGRESSBAR).append("' title='")
-            .append(join(percent, '%')).append("'> ").append("<div class='")
-            .append(C_PROGRESSBAR_VALUE).append("' style='")
-            .append(join("width:", percent, '%')).append("'> </div> </div>")
-            // History link
-            .append("\",\"<a href='").append(trackingURL).append("'>")
-            .append("History").append("</a>");
-        appsTableData.append("\"],\n");
-
-      } catch (Exception e) {
-        LOG.info(
-            "Cannot add application {}: {}", app.getAppId(), e.getMessage());
+
+    if (appsInfo != null) {
+      Collection<AppInfo> apps = appsInfo.getApps();
+      if (CollectionUtils.isNotEmpty(apps)) {
+        int numApps = apps.size();
+        int i = 0;
+        for (AppInfo app : apps) {
+          try {
+            String percent = String.format("%.1f", app.getProgress() * 100.0F);
+            String trackingURL =
+                app.getTrackingUrl() == null ? "#" : app.getTrackingUrl();
+
+            // AppID numerical value parsed by parseHadoopID in yarn.dt.plugins.js
+            appsTableData.append("[\"")
+                .append("<a href='").append(trackingURL).append("'>")
+                .append(app.getAppId()).append("</a>\",\"")
+                .append(escape(app.getUser())).append("\",\"")
+                .append(escape(app.getName())).append("\",\"")
+                .append(escape(app.getApplicationType())).append("\",\"")
+                .append(escape(app.getQueue())).append("\",\"")
+                .append(app.getPriority()).append("\",\"")
+                .append(app.getStartTime()).append("\",\"")
+                .append(app.getFinishTime()).append("\",\"")
+                .append(app.getState()).append("\",\"")
+                .append(app.getFinalStatus()).append("\",\"")
+                // Progress bar
+                .append("<br title='").append(percent).append("'> <div class='")
+                .append(C_PROGRESSBAR).append("' title='")
+                .append(join(percent, '%')).append("'> ").append("<div class='")
+                .append(C_PROGRESSBAR_VALUE).append("' style='")
+                .append(join("width:", percent, '%')).append("'> </div> </div>")
+                // History link
+                .append("\",\"<a href='").append(trackingURL).append("'>")
+                .append("History").append("</a>");
+            appsTableData.append("\"]\n");
+
+            if (i < numApps - 1) {
+              appsTableData.append(",");
+            }
+          } catch (Exception e) {
+            LOG.info("Cannot add application {}: {}", app.getAppId(), e.getMessage());
+          }
+          i++;

Review Comment:
   I read this part of the code carefully, the original code is reasonable, we should remove the extra comma outside the loop because we can't tell where the last comma is
   
   For example:
   We have 4 apps, A, B, C, D, and we expect the result to be [A, B, C, D], but when traversing the app list, the last app D fails, and we end up with [A ,B,C,], the last app D fails, we can't handle this situation inside the loop.
   
   But the readability of the original logic is not good, I refactored this part of the code.



-- 
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: common-issues-unsubscribe@hadoop.apache.org

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