siddharthteotia commented on code in PR #10123: URL: https://github.com/apache/pinot/pull/10123#discussion_r1073050440
########## pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/MultiStageOperator.java: ########## @@ -0,0 +1,49 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.pinot.query.runtime.operator; + +import java.util.List; +import org.apache.pinot.core.operator.BaseOperator; +import org.apache.pinot.query.runtime.blocks.TransferableBlock; +import org.slf4j.LoggerFactory; + + +public abstract class MultiStageOperator extends BaseOperator<TransferableBlock> implements AutoCloseable { + private static final org.slf4j.Logger LOGGER = LoggerFactory.getLogger(MultiStageOperator.class); + + // TODO: use the API public List<? extends Operator> getChildOperators() to merge two APIs. + @Override + public List<MultiStageOperator> getChildOperators() { + throw new UnsupportedOperationException(); + } + + // TODO: Ideally close() call should finish within request deadline. + // TODO: Consider passing deadline as part of the API. + @Override + public void close() { + for (MultiStageOperator op : getChildOperators()) { + try { + op.close(); Review Comment: I was thinking may be create a wrapper class called `AutoCloseables` For every operator / op chain etc, the closable resources that are acquired by that are added to this wrapper `AutoCloseables` where we can control the order in which we add Then close() can look like ``` List<AutoCloseable> autoCloseables = new ArrayList<>(); autoCloseables.add(res1); autoCloseables.add(res2); autoCloseables.add(res3); AutoCloseables.close(autoCloseables) ``` Now inside `AutoCloseables`, we can probably also do better exception handling / supression Example impl of `AutoCloseables` ``` public static void close(Iterable<? extends AutoCloseable> ac) throws Exception { if (ac == null) { return; } else if (ac instanceof AutoCloseable) { ((AutoCloseable) ac).close(); return; } Exception topLevelException = null; for (AutoCloseable closeable : ac) { try { if (closeable != null) { closeable.close(); } } catch (Exception e) { if (topLevelException == null) { topLevelException = e; } else if (e != topLevelException) { topLevelException.addSuppressed(e); } } } if (topLevelException != null) { throw topLevelException; } } ``` Don't mean to say we should do it now and the resource order cleanup may not even be a problem at this point. So maybe something to consider in future as resource mgmt and other things evolve. -- 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-unsubscr...@pinot.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org
