jira-importer commented on issue #538: URL: https://github.com/apache/curator/issues/538#issuecomment-2604692948
<i><a href="https://issues.apache.org/jira/secure/ViewProfile.jspa?name=cheddar";>cheddar</a>:</i> <p>The Branch generally looks fine to me. There are two things of note, though:</p> <p>There are two tests that I had created on TestPathChildrenCache for <a href="https://issues.apache.org/jira/browse/CURATOR-21"; title="PathChildrenCache consumes an entire thread all the time no matter what" class="issue-link" data-issue-key="CURATOR-21"><del>CURATOR-21</del></a> that I think would be helpful in verifying these changes, but I don't see them on the branch. One of the tests was in the main patch and one of them was in the "fix races" patch. I'm guessing the branch was done off of master (or something that hasn't had <a href="https://issues.apache.org/jira/browse/CURATOR-21"; title="PathChildrenCache consumes an entire thread all the time no matter what" class="issue-link" data-issue-key="CURATOR-21"><del>CURATOR-21</del></a> applied to it yet)?</p> <p>For the code itself, I'm not sure about the logic when closing the executor. It currently goes through all of the futures and tries to cancel them. That's normally fine, but when the cancel fails, it throws an exception out of the close. I think failure to cancel a task is actually a fairly common occurrence, so it'd probably be best to continue trying to close things. You could ignore the failures entirely (just log it) or throw an exception out at the end. I'd generally prefer to ignore and log, because I'm not sure that the person who calls close() can do much about the exceptional case of some tasks running even though they shouldn't be, but if you feel strongly about throwing an exception I won't hold it against you.</p> -- 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: dev-unsubscr...@curator.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
