belugabehr commented on a change in pull request #1055:
URL: https://github.com/apache/hive/pull/1055#discussion_r442264487
##########
File path:
service/src/java/org/apache/hive/service/cli/session/HiveSessionImpl.java
##########
@@ -757,14 +754,13 @@ public void close() throws HiveSQLException {
try {
acquire(true, false);
// Iterate through the opHandles and close their operations
- List<OperationHandle> ops = null;
- synchronized (opHandleSet) {
- ops = new ArrayList<>(opHandleSet);
- opHandleSet.clear();
- }
- for (OperationHandle opHandle : ops) {
+ List<OperationHandle> closedOps = new ArrayList<>();
+ for (OperationHandle opHandle : opHandleSet) {
operationManager.closeOperation(opHandle);
+ closedOps.add(opHandle);
}
+ opHandleSet.removeAll(closedOps);
Review comment:
Well, this is the `close()` method so presumably there are no more
coming in once this is called. Regardless, it won't matter. The `opHandleSet`
is synchronized here so it will remove all of the objects there were closed.
which is what the current implementation does too: if new ones come in after
the clear, they will just remain in the set. If we want to add some sort of
locking to prevent new entries from coming in after calling `close()`, that
would have to happen in a different ticket.
----------------------------------------------------------------
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:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscr...@hive.apache.org
For additional commands, e-mail: gitbox-h...@hive.apache.org
