[GitHub] [ozone] avijayanhwx commented on a change in pull request #1601: HDDS-4404. Datanode can go OOM when a Recon or SCM Server is very slow in processing reports

[GitBox <gi...@apache.org>]

avijayanhwx commented on a change in pull request #1601:
URL: https://github.com/apache/ozone/pull/1601#discussion_r527076699



##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/statemachine/StateContext.java
##########
@@ -190,16 +217,32 @@ void setShutdownGracefully() {
   public boolean getShutdownOnError() {
     return shutdownOnError;
   }
+
   /**
    * Adds the report to report queue.
    *
    * @param report report to be added
    */
   public void addReport(GeneratedMessage report) {
     if (report != null) {
-      synchronized (reports) {
-        for (InetSocketAddress endpoint : endpoints) {
-          reports.get(endpoint).add(report);
+      final String reportType = report.getDescriptorForType().getFullName();
+      for (InetSocketAddress endpoint : endpoints) {
+        // Check report type
+        if (reportType.equals(containerReportsProtoName)) {
+          containerReport = report;
+        } else if (reportType.equals(nodeReportProtoName)) {
+          nodeReport = report;
+        } else if (reportType.equals(pipelineReportsProtoName)) {
+          pipelineReport = report;
+        } else if (reportType.equals(commandStatusReportsProtoName) ||
+            reportType.equals(incrementalContainerReportProtoName)) {
+          // report type is CommandStatusReports or IncrementalContainerReport
+          synchronized (reports) {
+            reports.get(endpoint).add(report);

Review comment:
       I see that this is a new logical condition here. When a new report publisher is added, explicit changes are needed here. If not, we can keep Line 237 as a simple else.

##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/states/endpoint/HeartbeatEndpointTask.java
##########
@@ -184,15 +181,30 @@ private void addReports(SCMHeartbeatRequestProto.Builder requestBuilder) {
     for (GeneratedMessage report :
         context.getAllAvailableReports(rpcEndpoint.getAddress())) {
       String reportName = report.getDescriptorForType().getFullName();
+      // Example reportName = hadoop.hdds.NodeReportProto
+
       for (Descriptors.FieldDescriptor descriptor :
           SCMHeartbeatRequestProto.getDescriptor().getFields()) {
+
         String heartbeatFieldName = descriptor.getMessageType().getFullName();
+        // Possible heartbeatFieldName =
+        //  hadoop.hdds.DatanodeDetailsProto
+        //  hadoop.hdds.NodeReportProto
+        //  hadoop.hdds.ContainerReportsProto
+        //  hadoop.hdds.IncrementalContainerReportProto
+        //  hadoop.hdds.CommandStatusReportsProto
+        //  hadoop.hdds.ContainerActionsProto
+        //  hadoop.hdds.PipelineActionsProto
+        //  hadoop.hdds.PipelineReportsProto
         if (heartbeatFieldName.equals(reportName)) {
           if (descriptor.isRepeated()) {
             requestBuilder.addRepeatedField(descriptor, report);
           } else {
             requestBuilder.setField(descriptor, report);
           }
+          // TODO: We can exit loop early here since we have a match already,
+          //  right? Double check.
+          break;

Review comment:
       Yes, it looks like we can break out here.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org