svalaskevicius commented on PR #15312: URL: https://github.com/apache/iceberg/pull/15312#issuecomment-3991006171
> > I'm a bit wary of the nondeterministic state of this PR/task - esp with the new parallel PR out there :) would it be possible to have a think and set out a single list of changes pending before it can be considered complete/good enough? > > I understand that this feels uncertain, but as long as we discover open issues, we need to address them. It's a good idea to make a plan and update it over time. Both contributor and reviewer can do that. Let me try to formulate the acceptance criteria thanks for this reply - it is reassuring, and I agree wrt the new uncovered issues. I'll respond below inline too: > 1. Centrally manage all static thread pools and allow them to be shut down manually :+1: I believe this is done (subject to review ofc!) > 2. Keep the existing behavior in place to shut down thread pools on JVM shutdown :+1: I believe this is done, with an exception the the shutdown duration changed in `AuthRefreshPoolHolder` (which I copied from the other PR) > 3. Clean code and tests :+1: I believe this is done -- for the code part > > also, what tests do you have in mind? > > There are several things we can test: > > * Verify static thread pools are registered ok, I can add this > * Verify shutdown works as intended > > * Verify shutdown hook gets added / removed these might need some thinking how to void stopping the static thread pools so that other tests could continue running by construction. I suppose I can extract a non static class similar to the other PR. would not be a big change I believe -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
