Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ValidationExceptionHandler. #202

Merged
merged 1 commit into from Jan 17, 2022
Merged

Conversation

dfish3r
Copy link
Member

@dfish3r dfish3r commented Nov 4, 2021

Pooling implementation would benefit from handling validation exceptions on checkout.
Provide an implementation that retries getting a connection.
Bring back ValidationException and ActivationException for general API usage.

Copy link
Member

@serac serac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like useful functionality, but I'm concerned it risks adding to configuration complexity that I would characterize as already fairly high for this library. I would recommend that you consider simply making retry the default behavior when validateOnCheckout is enabled. I think that's what most people want from a pooling library: always return good connections.

Pooling implementation would benefit from handling validation exceptions on checkout.
Provide a default implementation that retries getting a connection.
Bring back ValidationException and ActivationException for general API usage.
@dfish3r dfish3r force-pushed the validation-exception-handler branch from aa4fdd4 to f6223e8 Compare January 12, 2022 23:07
@serac serac merged commit 5cbc01b into master Jan 17, 2022
@vladimir-mencl-eresearch

Hi @dfish3r ,

Thanks for your input in the discussion at https://shibboleth.atlassian.net/browse/IDP-1898

I'm trying to assess whether this PR would actually fix the issue I'm facing (described in the above link), where:

  • a connection pool is configured against two LDAP servers
  • uses the ACTIVE_PASSIVE connection strategy
  • and the first server suddenly becomes unresponsive

What I see as desired behaviour is retrying until a connection is established to the other server.

With this implementation (retrying up to getMaxPoolSize() times), in the extreme case, when the pool is at the maximum size, with all connections established to the first server, the retries would stop one step short of reaching the other server.

When the pool is not at the maximum size, after using all connections in the pool, there would be an attempt to establish a connection with the other server.

Would you consider changing the number of retries to getMaxPoolSize() + 1 to accommodate also the situation when the pool is at the maximum size?

Cheers,
Vlad

@dfish3r
Copy link
Member Author

dfish3r commented Jan 20, 2022

One of the features in v2 is auto-reconnect. So in the nominal case, when your active server goes down, connections should attempt a reconnect in the background to the passive server. But let's assume some pesky load balancer that doesn't send TCP resets. In that case you would be dependent on the pool validation to reconnect.

I think you make a good argument for maxSize +1 and an early version of this code worked the same way. So I'll make that change. Note that the code still respects the pool block wait time. So for large pools you may not have enough time to cycle all the connections out of the pool.

@vladimir-mencl-eresearch

Thanks! Yes, I'm aware the timeout might not still cut it - but this would be a step in the right direction. Thanks for that!

dfish3r added a commit that referenced this pull request Jan 21, 2022
Guarantee one new connection is added to the pool in the worst case.
See #202.
@dfish3r
Copy link
Member Author

dfish3r commented Jan 21, 2022

Fixed in e60a233

@vladimir-mencl-eresearch

Thanks!

dfish3r added a commit that referenced this pull request Jan 27, 2022
@dfish3r dfish3r deleted the validation-exception-handler branch November 8, 2023 22:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants