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

Validate server types before rpc requests #5152

Closed
wants to merge 1 commit into from

Conversation

cshannon
Copy link
Contributor

@cshannon cshannon commented Dec 8, 2024

This is a follow on to #5102

Before making requests such as getScans() or getCompactions() the server types for each server will be validated to make sure it is a valid type. The requests are multi-threaded so this pre-check validation prevents making a bunch of unnecessary RPC server calls if one of the types would fail validation. This change also fixes the getScans() and getCompactions() API so that when a server is invalid the correct exception is thrown and not wrapped in an AccumuloException.

@cshannon
Copy link
Contributor Author

cshannon commented Dec 8, 2024

@dlmarion and @keith-turner - This fixes the failed test mentioned here

The cause was that with the refactoring to make everything multi-threaded all the validation was being done for each server in a new thread and the exception was being wrapped in ExecutionException and then caught and unwrapped and rethrown as AccumuloException.

I could have fixed the test to handle this but instead I thought it made a lot more sense to do a pre-validation before the thread pool was constructed and any RPC calls are made. The time it takes to iterate through even a few thousand servers just to check the type and fail fast before making any RPC calls is going to be trivial and it prevents the case where all the requests get made and eat up a bunch of resources when one of the servers are invalid which causes the whole thing to eventually fail anyways.

Besides this new validation potentially saving resources, it also has the nice side effect of fixing the test failure as the IllegalArgumentException is now properly thrown again and not wrapped.

This is a follow on to apache#5102

Before making requests such as getScans() or getCompactions() the server
types for each server will be validated to make sure it is a valid type.
The requests are multi-threaded so this pre-check validation prevents
making a bunch of unnecessary RPC server calls if one of the types would
fail validation. This change also fixes the getScans() and
getCompactions() API so that when a server is invalid the correct
exception is thrown and not wrapped in an AccumuloException.
@cshannon cshannon self-assigned this Dec 8, 2024
@cshannon cshannon added this to the 4.0.0 milestone Dec 8, 2024
@cshannon
Copy link
Contributor Author

cshannon commented Dec 8, 2024

Closing as I didn't realize #5150 was opened and this is basically a duplicate

@cshannon cshannon closed this Dec 8, 2024
@ctubbsii ctubbsii removed this from the 4.0.0 milestone Dec 9, 2024
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.

2 participants