You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2022/06/04 15:54:55 UTC
[GitHub] [iceberg] bryanck opened a new pull request, #4958: Shutdown refresh token executor service during REST catalog client close
bryanck opened a new pull request, #4958:
URL: https://github.com/apache/iceberg/pull/4958
This PR adds a shutdown to the refresh token executor service when the REST catalog client is closed.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] bryanck commented on a diff in pull request #4958: Shutdown refresh token executor service during REST catalog client close
Posted by GitBox <gi...@apache.org>.
bryanck commented on code in PR #4958:
URL: https://github.com/apache/iceberg/pull/4958#discussion_r889691437
##########
core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java:
##########
@@ -369,6 +369,7 @@ public void close() throws IOException {
ScheduledExecutorService service = refreshExecutor;
this.refreshExecutor = null;
+ service.shutdown();
Review Comment:
I'm thinking it is better to throw, so if the `shudown()` fails, it won't proceed to the `awaitTermination()`
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on a diff in pull request #4958: Shutdown refresh token executor service during REST catalog client close
Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #4958:
URL: https://github.com/apache/iceberg/pull/4958#discussion_r889628755
##########
core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java:
##########
@@ -369,6 +369,7 @@ public void close() throws IOException {
ScheduledExecutorService service = refreshExecutor;
this.refreshExecutor = null;
+ service.shutdown();
Review Comment:
Agreed. This is sufficient for now in my opinion. I also think the example from the javadoc might be overkill (if anything the results will simply be processed and not returned to anything by the catalog service, which happens all the time during shutdowns etc).
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on a diff in pull request #4958: Shutdown refresh token executor service during REST catalog client close
Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #4958:
URL: https://github.com/apache/iceberg/pull/4958#discussion_r889628755
##########
core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java:
##########
@@ -369,6 +369,7 @@ public void close() throws IOException {
ScheduledExecutorService service = refreshExecutor;
this.refreshExecutor = null;
+ service.shutdown();
Review Comment:
Agreed. This is sufficient for now in my opinion. I also think the example from the javadoc might be overkill (if anything the results will simply be processed and not returned to anything by the server, which happens all the time during shutdown etc).
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] bryanck commented on pull request #4958: Shutdown refresh token executor service during REST catalog client close
Posted by GitBox <gi...@apache.org>.
bryanck commented on PR #4958:
URL: https://github.com/apache/iceberg/pull/4958#issuecomment-1146805796
The way it is now is similar to the shutdown in `RewriteDataFilesCommitManager.java`.
One minor thing is that the client is being closed before the executor is shut down, so any refresh tasks in progress will most likely fail. Awaiting termination might not be that useful. The refreshed token isn't going to be used if the catalog is being closed AFAIK. Alternatively, closing the client could come after the awaitTermination if that's a concern.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] bryanck commented on a diff in pull request #4958: Shutdown refresh token executor service during REST catalog client close
Posted by GitBox <gi...@apache.org>.
bryanck commented on code in PR #4958:
URL: https://github.com/apache/iceberg/pull/4958#discussion_r889628612
##########
core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java:
##########
@@ -369,6 +369,7 @@ public void close() throws IOException {
ScheduledExecutorService service = refreshExecutor;
this.refreshExecutor = null;
+ service.shutdown();
Review Comment:
My thought is that a try/catch block might be overkill, but I can add that if desired
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue merged pull request #4958: Shutdown refresh token executor service during REST catalog client close
Posted by GitBox <gi...@apache.org>.
rdblue merged PR #4958:
URL: https://github.com/apache/iceberg/pull/4958
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on a diff in pull request #4958: Shutdown refresh token executor service during REST catalog client close
Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #4958:
URL: https://github.com/apache/iceberg/pull/4958#discussion_r889628278
##########
core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java:
##########
@@ -369,6 +369,7 @@ public void close() throws IOException {
ScheduledExecutorService service = refreshExecutor;
this.refreshExecutor = null;
+ service.shutdown();
Review Comment:
Good catch.
One question: do we potentially want to do this inside of a try block? [Looking at the javadocs](https://docs.oracle.com/javase/8/docs/api/java/util/concurrent/ExecutorService.html#shutdown--), it seems like the only exception it can throw is `SecurityException` which likely shouldn't happen if the executor service was started.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on pull request #4958: Shutdown refresh token executor service during REST catalog client close
Posted by GitBox <gi...@apache.org>.
rdblue commented on PR #4958:
URL: https://github.com/apache/iceberg/pull/4958#issuecomment-1146856299
> One minor thing is that the client is being closed before the executor is shut down, so any refresh tasks in progress will most likely fail.
Why not move the thread pool shutdown before closing the client?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on pull request #4958: Shutdown refresh token executor service during REST catalog client close
Posted by GitBox <gi...@apache.org>.
rdblue commented on PR #4958:
URL: https://github.com/apache/iceberg/pull/4958#issuecomment-1146856360
Thanks, @bryanck!
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org