adoroszlai commented on code in PR #5791:
URL: https://github.com/apache/ozone/pull/5791#discussion_r1480162258


##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java:
##########
@@ -2358,7 +2366,7 @@ private KeyOutputStream.Builder createKeyOutputStream(
         .enableUnsafeByteBufferConversion(unsafeByteBufferConversion)
         .setConfig(clientConfig)
         .setAtomicKeyCreation(isS3GRequest.get())
-        .setClientMetrics(clientMetrics)
+        
.setBlockOutPutStreamResourceProvider(blockOutPutStreamResourceProvider)

Review Comment:
   > closure accesses a lot of external variables, which could be a potential 
thread-safety issue
   
   I have the following concerns:
   
   1. Are the same external variables protected by the same object elsewhere?  
If not, there is no point in making this synchronized.
   2. `validateResponse` only accesses an atomic reference and the response 
object itself.  Neither of those need to be synced.
   3. Protobuf conversion (`BlockID.getFromProtobuf`) should not be performed 
while holding the lock.
   4. `(this.)blockID` is also atomic, does not need the lock.
   5. logging should not be performed while holding the lock.
   
   So in the end I think only `updateCommitInfo` needs to be protected.



-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to