[GitHub] [druid] rohangarg commented on a diff in pull request #13974: OOM fix for running MSQ jobs with `intermediateSuperSorterStorageMaxLocalBytes` set

["rohangarg (via GitHub)" <gi...@apache.org>]

rohangarg commented on code in PR #13974:
URL: https://github.com/apache/druid/pull/13974#discussion_r1147781573


##########
processing/src/main/java/org/apache/druid/frame/channel/ComposingWritableFrameChannel.java:
##########
@@ -105,6 +100,16 @@ public void write(FrameWithPartition frameWithPartition) throws IOException
     }
   }
 
+  private void convertChannelSuppliersToReadOnly(int index)

Review Comment:
   could create a common interface for `PartitionedOutputChannel` and `OutputChannel` which encapsulates `convertToReadOnly` - that way we can only give a single instance of the common interface to this object. 
   Also, is there a need to have `convertToReadOnly` in the `ComposingOutputChannelFactory` now that we close the channels as soon as the writes for them are done? cc @LakshSingla 
   These things could be followups.



##########
processing/src/main/java/org/apache/druid/frame/channel/ComposingWritableFrameChannel.java:
##########
@@ -118,14 +123,17 @@ public void close() throws IOException
   {
     if (currentIndex < writableChannelSuppliers.size()) {
       writableChannelSuppliers.get(currentIndex).get().close();
+      convertChannelSuppliersToReadOnly(currentIndex);
       currentIndex = writableChannelSuppliers.size();
     }
   }
 
   @Override
   public boolean isClosed()
   {
-    return currentIndex == writableChannelSuppliers.size();
+    return currentIndex == writableChannelSuppliers.size() || writableChannelSuppliers.get(currentIndex)

Review Comment:
   Can you please explain the reason for adding the extra check?



-- 
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: commits-unsubscribe@druid.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org