Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
AJ-1371: link snapshots during AnVIL pfb import #386
AJ-1371: link snapshots during AnVIL pfb import #386
Changes from 17 commits
2e519ac
5ec3af3
fd0d8c4
712365f
56ebee0
456937c
4e872f0
4f8e816
95322c8
6477820
8ebd5cb
83ade66
64f9e6e
40b8076
112e485
bec0aeb
2abf7ec
b040220
81a164b
167fc4b
77bdfcb
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the arg comment ❤️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possibly a place for an
Optional
instead of anull
? (disclosure: I generally steer clear of nulls in Java if possible)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm gonna push back here - I think that's sound advice to use
Optional
instead ofnull
for public methods, but this is a private method used only within the class. If it returnedOptional
, it would require additional steps inside ofexecuteInternal()
. Whereas now we can use one step.filter(Objects::nonNull)
, we'd need to change to.filter(Optional::isPresent).map(Optional::get)
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds fine, particularly making the distinction between public and private APIs. I have a hunch that there'd be some optional-friendly predicate that could stand in for (and have equivalent terseness to) what's already there, but don't know off the top of my head what it'd be so carry on!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any sensible way to fail faster if we'll exceed the hard limit? Maybe by pushing the hard limit enforcement down to WSM?
Feels like this approach ends up doing a whole bunch of wasted work and would potentially fail slowly and cause resource waste/exhaustion when paired with some form of retries.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only other approach I can think of is to pick a number (maybe not as high as 10000), and make a single request to WSM for that many resources - don't attempt to paginate. Do you think that would be better?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think if by your guess this is a really really unlikely scenario that it's maybe not something to worry about and this might be the perfect safeguard...wait till it happens, then worry about it.
I don't think I understand well enough how these snapshots get written to WSM, but the only thing that I think might be better is if we could somehow head them off at write time (into WSM) rather than here at read time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another possible place for an
Optional
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same pushback here - as a non-public method (it's only protected instead of private so I can unit-test it), I'm optimizing for performance instead of a good API - this one is also used directly as a stream mapping function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The way
PfbImportException
gets thrown (for blowing the hard limit), and the fact that it subclassesIllegalArgumentException
make it seem more like some kind of client error than an internal error to me. Should this be a different HTTP status?That said, I think we'd want to know about these early on as we learn how this feature gets used, so if it makes more sense that it's an 'us' problem than a client problem, then you can keep it as
INTERNAL_SERVER_ERROR
...I just wonder if it should subclass a different kind of exception in that case.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The use of
IllegalArgumentException
here was a copy/paste mistake on my part, I have changed to extendRuntimeException
!I do consider this an "us" problem. Even though I think it highly unlikely that a workspace will have > 10000 snapshot references, it's our fault that we can't handle such a case, so I agree on keeping it as
INTERNAL_SERVER_ERROR
.fwiw, I originally wrote the loop inside
listAllSnapshots
as awhile(true)
loop, but I got nervous about infinity and decided to add a sanity check and thus imposed a hard limit.