[GitHub] [pinot] walterddr commented on a diff in pull request #9967: [POC] Operator resource management

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

walterddr commented on code in PR #9967:
URL: https://github.com/apache/pinot/pull/9967#discussion_r1047811993


##########
pinot-core/src/main/java/org/apache/pinot/core/common/Operator.java:
##########
@@ -41,6 +41,8 @@
    */
   T nextBlock();
 
+  void close();
+

Review Comment:
   let's dont add the close method in the main interface. as the v1 engine operator API doesn't actually hold on to any native closable resources (they are held by either the executor or the DataTable reducer)
   
   Instead, I suggest 2 alternatives:
   1. we create a `V2Operator` or `IntermediateStageOperator` API and add the `close()` method there. 
   2. make the closable V2 operators (mailbox send, mailbox receive, etc) implement Closeable 
   ```
   public class MailboxSendOperator extends BaseOperator<TransferableBlock> implements Closable {
   ...
   ```
   
   these 2 alternative ways: (1) it provides clean separation of concern; (2) doesn't effectively refactored hundred lines of code; (3) doesn't require us to move the non-native closable resources in V1 engine over to the proper close() method --> thus avoid confusion in future code maintenance. 
   
   personally I prefer the 2nd option. 



-- 
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@pinot.apache.org

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


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