[GitHub] [ozone] adoroszlai commented on a change in pull request #3110: HDDS-6334. Remove ContainerID to Proto to ContainerID conversion in ContainerStateManagerImpl

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

adoroszlai commented on a change in pull request #3110:
URL: https://github.com/apache/ozone/pull/3110#discussion_r811963462



##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerStateManagerImpl.java
##########
@@ -326,11 +326,11 @@ public void addContainer(final ContainerInfoProto containerInfo)
   }
 
   @Override
-  public boolean contains(final HddsProtos.ContainerID id) {
+  public boolean contains(ContainerID id) {
     lock.readLock().lock();
     try {
       // TODO: Remove the protobuf conversion after fixing ContainerStateMap.

Review comment:
       TODO: remove the TODO after addressing it. ;)

##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerManagerImpl.java
##########
@@ -278,18 +275,17 @@ private ContainerInfo allocateContainer(final Pipeline pipeline,
         .build();
     containerStateManager.addContainer(containerInfo);
     scmContainerManagerMetrics.incNumSuccessfulCreateContainers();
-    return containerStateManager.getContainer(containerID.getProtobuf());
+    return containerStateManager.getContainer(containerID);
   }
 
   @Override
-  public void updateContainerState(final ContainerID id,
+  public void updateContainerState(final ContainerID cid,
                                    final LifeCycleEvent event)
       throws IOException, InvalidStateTransitionException {
-    final HddsProtos.ContainerID cid = id.getProtobuf();
     lock.lock();
     try {
       if (containerExist(cid)) {
-        containerStateManager.updateContainerState(cid, event);
+        containerStateManager.updateContainerState(cid.getProtobuf(), event);

Review comment:
       Is it worth moving the `getProtobuf()` call inside the locked section, to avoid calls for nonexistent containers?

##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerManagerImpl.java
##########
@@ -414,13 +408,12 @@ public void notifyContainerReportProcessing(final boolean isFullReport,
   }
 
   @Override
-  public void deleteContainer(final ContainerID id)
+  public void deleteContainer(final ContainerID cid)
       throws IOException {
-    final HddsProtos.ContainerID cid = id.getProtobuf();
     lock.lock();
     try {
       if (containerExist(cid)) {
-        containerStateManager.removeContainer(cid);
+        containerStateManager.removeContainer(cid.getProtobuf());

Review comment:
       Same question here on lock vs. unnecessary call.




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

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