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

refactor: introduce Store(Item)?NotFoundException classes #360

Merged
merged 5 commits into from
Jun 20, 2024

Conversation

malandis
Copy link
Contributor

This PR detects when a NOT_FOUND gRPC error was specific to the
storage client or not. For the storage client NOT_FOUND, there are
two cases: the store was not found and the item was not found.

To distinguish these two cases we use string matching on the
grpcException message.

This PR detects when a `NOT_FOUND` gRPC error was specific to the
storage client or not. For the storage client `NOT_FOUND`, there are
two cases: the store was not found and the item was not found.

To distinguish these two cases we use string matching on the
grpcException message.
@malandis
Copy link
Contributor Author

Not terribly important or urgent: we may consider organizing the various not found exception classes as follows:

  • a base class ResourceNotFoundException, with subclasses
  • CacheResourceNotFoundException and StoreResourceNotFoundException
  • the latter derives StoreItemNotFoundException and StoreNotFoundException

@malandis malandis requested review from nand4011, cprice404 and a team June 20, 2024 18:39
@malandis
Copy link
Contributor Author

malandis commented Jun 20, 2024

Naming choices I made:

  • StoreNotFoundException for when a store is not found
  • StoreItemNotFoundException for when the item is not found. I feel that prefixing with Store reduces ambiguity vs a cache item for users typing in an IDE. Alternatively we could use StorageItemNotFoundException which, while better English, leaves the prefix inconsistent. For grouping purposes a single prefix is a more convenient DX.

nand4011
nand4011 previously approved these changes Jun 20, 2024
Copy link
Contributor

@nand4011 nand4011 left a comment

Choose a reason for hiding this comment

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

LGTM. We'll have to be careful about building an exception hierarchy that includes cache exceptions, since we could technically introduce breaking changes if a call starts returning a different error type than before.

The metadata should come from the exception trailers, not the request
metadata.
@malandis malandis requested a review from nand4011 June 20, 2024 20:10
Comment on lines +54 to +57
if (errorCause == null) {
// TODO remove once control service is updated to send "err" in metadata
errorCause = grpcException.getMessage();
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will update this once the server is up to date.

Comment on lines +85 to +92
if (errorCause.contains("element_not_found")) {
return new StoreItemNotFoundException(grpcException, errorDetails);
// TODO change once control service is updated to send "Store with name" in metadata
} else if (errorCause.contains("Store with name")) {
return new StoreNotFoundException(grpcException, errorDetails);
} else {
return new NotFoundException(grpcException, errorDetails);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will update these once the server is up to date.

@malandis malandis requested a review from a team June 20, 2024 20:13
@malandis malandis merged commit 81b25c2 into main Jun 20, 2024
5 checks passed
@malandis malandis deleted the chore/storage-client-errors branch June 20, 2024 20:22
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