amogh-jahagirdar commented on code in PR #10140:
URL: https://github.com/apache/iceberg/pull/10140#discussion_r1565848626
##########
core/src/main/java/org/apache/iceberg/ClientPoolImpl.java:
##########
@@ -56,26 +67,36 @@ public <R> R run(Action<R, C, E> action, boolean retry)
throws E, InterruptedExc
C client = get();
try {
return action.run(client);
-
} catch (Exception exc) {
- if (retry && isConnectionException(exc)) {
- try {
- client = reconnect(client);
- } catch (Exception ignored) {
- // if reconnection throws any exception, rethrow the original failure
- throw reconnectExc.cast(exc);
- }
-
- return action.run(client);
+ if (!isConnectionException(exc)) {
+ throw exc;
}
- throw exc;
+ return retryAction(action, exc, client);
} finally {
release(client);
}
}
+ private <R> R retryAction(Action<R, C, E> action, Exception originalFailure,
C client) throws E {
+ int retryAttempts = 0;
+ while (retryAttempts < maxRetries) {
+ try {
+ C reconnectedClient = reconnect(client);
+ return action.run(reconnectedClient);
+ } catch (Exception exc) {
+ if (isConnectionException(exc)) {
+ retryAttempts++;
+ } else {
+ throw reconnectExc.cast(originalFailure);
+ }
+ }
+ }
+
+ throw reconnectExc.cast(originalFailure);
Review Comment:
So my thoughts @jbonofre:
1.) C3P0 is a great pooling library! Less familiar with DBCP2 but I'm sure
it's in a similar vein. We should leverage such libraries where we can to avoid
reimplementing this sort of complex, redundant logic.
2.) That being said, given that long term we would be separating out the
catalog into a separate project from what we discussed in the past community
sync, the question comes down to (I think) :
do we want to do the work of adding the C3P0 dependency, changing the code
to work with that at this point in time, or do we want to do it after a
migration to a separate project?
I fall in the latter camp since then we'd need to maintain the
implementation until the migration were to happen; and we've already reached a
point where it's (imo) a lot for the Iceberg project itself to maintain. The
improvements for multiple backends just working is great, since that's what
people expect from JDBC but again I'm not sure if we want to that upfront here
or just a separate catalog project which we would do just prior to a 2.0
version of Iceberg
--
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]
