You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by GitBox <gi...@apache.org> on 2021/06/15 19:04:06 UTC

[GitHub] [ozone] umamaheswararao opened a new pull request #2341: HDDS-5350 : Add allocate block support in MockOmTransport

umamaheswararao opened a new pull request #2341:
URL: https://github.com/apache/ozone/pull/2341


   ## What changes were proposed in this pull request?
   
   Added allocateBlock API in MockOmTransport.
   
   ## What is the link to the Apache JIRA
   
   https://issues.apache.org/jira/browse/HDDS-5350
   
   ## How was this patch tested?
   
   Added a test which triggers allocateBlock. Without source changes, test fails and with source changes test passed.
   


-- 
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

[GitHub] [ozone] elek commented on a change in pull request #2341: HDDS-5350 : Add allocate block support in MockOmTransport

Posted by GitBox <gi...@apache.org>.
elek commented on a change in pull request #2341:
URL: https://github.com/apache/ozone/pull/2341#discussion_r657193657



##########
File path: hadoop-ozone/client/src/test/java/org/apache/hadoop/ozone/client/MockOmTransport.java
##########
@@ -116,13 +117,24 @@ public OMResponse submitRequest(OMRequest payload) throws IOException {
       return response(payload,
           r -> r.setServiceListResponse(
               serviceList(payload.getServiceListRequest())));
+    case AllocateBlock:
+      return response(payload, r -> r.setAllocateBlockResponse(
+          allocateBlock(payload.getAllocateBlockRequest())));
     default:
       throw new IllegalArgumentException(
           "Mock version of om call " + payload.getCmdType()
               + " is not yet implemented");
     }
   }
 
+  private OzoneManagerProtocolProtos.AllocateBlockResponse allocateBlock(
+      OzoneManagerProtocolProtos.AllocateBlockRequest allocateBlockRequest) {
+    return OzoneManagerProtocolProtos.AllocateBlockResponse.newBuilder()
+        .setKeyLocation(
+            blockAllocator.allocateBlock(allocateBlockRequest.getKeyArgs())
+                .iterator().next()).build();

Review comment:
       It's possible to return with multiple key locations if the keyArgs contains a size which is huge enough. Today it's not supported by the block allocated, but looks to be safer to add all the results to the `setKeyLocation`...




-- 
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

[GitHub] [ozone] sodonnel commented on a change in pull request #2341: HDDS-5350 : Add allocate block support in MockOmTransport

Posted by GitBox <gi...@apache.org>.
sodonnel commented on a change in pull request #2341:
URL: https://github.com/apache/ozone/pull/2341#discussion_r656993110



##########
File path: hadoop-ozone/client/src/test/java/org/apache/hadoop/ozone/client/MockOmTransport.java
##########
@@ -116,13 +117,24 @@ public OMResponse submitRequest(OMRequest payload) throws IOException {
       return response(payload,
           r -> r.setServiceListResponse(
               serviceList(payload.getServiceListRequest())));
+    case AllocateBlock:
+      return response(payload, r -> r.setAllocateBlockResponse(
+          allocateBlock(payload.getAllocateBlockRequest())));
     default:
       throw new IllegalArgumentException(
           "Mock version of om call " + payload.getCmdType()
               + " is not yet implemented");
     }
   }
 
+  private OzoneManagerProtocolProtos.AllocateBlockResponse allocateBlock(
+      OzoneManagerProtocolProtos.AllocateBlockRequest allocateBlockRequest) {
+    return OzoneManagerProtocolProtos.AllocateBlockResponse.newBuilder()
+        .setKeyLocation(
+            blockAllocator.allocateBlock(allocateBlockRequest.getKeyArgs())
+                .iterator().next()).build();

Review comment:
       Looking at SinglePipelineBlockAllocator, I am not sure it can return multiple results, or that you can request multiple blocks in one go from the AllocateBlockRequest. However it does return an Iterable<>, so you have to call next on it as you have done.
   
   The response AllocateBlockResponse object just contains a single "KeyLocation", which contains a single "BlockID" and "Pipeline". Therefore I *think* your code is correct and it does not need a loop 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

[GitHub] [ozone] umamaheswararao commented on a change in pull request #2341: HDDS-5350 : Add allocate block support in MockOmTransport

Posted by GitBox <gi...@apache.org>.
umamaheswararao commented on a change in pull request #2341:
URL: https://github.com/apache/ozone/pull/2341#discussion_r657171259



##########
File path: hadoop-ozone/client/src/test/java/org/apache/hadoop/ozone/client/MockOmTransport.java
##########
@@ -116,13 +117,24 @@ public OMResponse submitRequest(OMRequest payload) throws IOException {
       return response(payload,
           r -> r.setServiceListResponse(
               serviceList(payload.getServiceListRequest())));
+    case AllocateBlock:
+      return response(payload, r -> r.setAllocateBlockResponse(
+          allocateBlock(payload.getAllocateBlockRequest())));
     default:
       throw new IllegalArgumentException(
           "Mock version of om call " + payload.getCmdType()
               + " is not yet implemented");
     }
   }
 
+  private OzoneManagerProtocolProtos.AllocateBlockResponse allocateBlock(
+      OzoneManagerProtocolProtos.AllocateBlockRequest allocateBlockRequest) {
+    return OzoneManagerProtocolProtos.AllocateBlockResponse.newBuilder()
+        .setKeyLocation(
+            blockAllocator.allocateBlock(allocateBlockRequest.getKeyArgs())
+                .iterator().next()).build();

Review comment:
       oh yeah. Looks like your are right. API name says keyLocation, but not keyLocations. 




-- 
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

[GitHub] [ozone] umamaheswararao commented on a change in pull request #2341: HDDS-5350 : Add allocate block support in MockOmTransport

Posted by GitBox <gi...@apache.org>.
umamaheswararao commented on a change in pull request #2341:
URL: https://github.com/apache/ozone/pull/2341#discussion_r656406227



##########
File path: hadoop-ozone/client/src/test/java/org/apache/hadoop/ozone/client/MockOmTransport.java
##########
@@ -116,13 +117,24 @@ public OMResponse submitRequest(OMRequest payload) throws IOException {
       return response(payload,
           r -> r.setServiceListResponse(
               serviceList(payload.getServiceListRequest())));
+    case AllocateBlock:
+      return response(payload, r -> r.setAllocateBlockResponse(
+          allocateBlock(payload.getAllocateBlockRequest())));
     default:
       throw new IllegalArgumentException(
           "Mock version of om call " + payload.getCmdType()
               + " is not yet implemented");
     }
   }
 
+  private OzoneManagerProtocolProtos.AllocateBlockResponse allocateBlock(
+      OzoneManagerProtocolProtos.AllocateBlockRequest allocateBlockRequest) {
+    return OzoneManagerProtocolProtos.AllocateBlockResponse.newBuilder()
+        .setKeyLocation(
+            blockAllocator.allocateBlock(allocateBlockRequest.getKeyArgs())
+                .iterator().next()).build();

Review comment:
       Let me add loop here for all KeyArgs available




-- 
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

[GitHub] [ozone] elek commented on a change in pull request #2341: HDDS-5350 : Add allocate block support in MockOmTransport

Posted by GitBox <gi...@apache.org>.
elek commented on a change in pull request #2341:
URL: https://github.com/apache/ozone/pull/2341#discussion_r657257138



##########
File path: hadoop-ozone/client/src/test/java/org/apache/hadoop/ozone/client/MockOmTransport.java
##########
@@ -116,13 +117,24 @@ public OMResponse submitRequest(OMRequest payload) throws IOException {
       return response(payload,
           r -> r.setServiceListResponse(
               serviceList(payload.getServiceListRequest())));
+    case AllocateBlock:
+      return response(payload, r -> r.setAllocateBlockResponse(
+          allocateBlock(payload.getAllocateBlockRequest())));
     default:
       throw new IllegalArgumentException(
           "Mock version of om call " + payload.getCmdType()
               + " is not yet implemented");
     }
   }
 
+  private OzoneManagerProtocolProtos.AllocateBlockResponse allocateBlock(
+      OzoneManagerProtocolProtos.AllocateBlockRequest allocateBlockRequest) {
+    return OzoneManagerProtocolProtos.AllocateBlockResponse.newBuilder()
+        .setKeyLocation(
+            blockAllocator.allocateBlock(allocateBlockRequest.getKeyArgs())
+                .iterator().next()).build();

Review comment:
       Oh, thanks for the offline discussion with you, I got it. Pure `allocateBlock` supports one `keyLocation`. Open key supports multiple location. Strange,  but this is what we have. Thanks to help me learninig 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.

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

[GitHub] [ozone] umamaheswararao merged pull request #2341: HDDS-5350 : Add allocate block support in MockOmTransport

Posted by GitBox <gi...@apache.org>.
umamaheswararao merged pull request #2341:
URL: https://github.com/apache/ozone/pull/2341


   


-- 
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