AJ-1371: link snapshots during AnVIL pfb import #386
Conversation
service/src/main/java/org/databiosphere/workspacedataservice/dataimport/PfbQuartzJob.java
Outdated
Show resolved
Hide resolved
service/src/main/java/org/databiosphere/workspacedataservice/dataimport/PfbQuartzJob.java
Outdated
Show resolved
Hide resolved
service/src/main/java/org/databiosphere/workspacedataservice/dataimport/PfbQuartzJob.java
Show resolved
Hide resolved
service/src/main/java/org/databiosphere/workspacedataservice/dataimport/PfbQuartzJob.java
Outdated
Show resolved
Hide resolved
| } | ||
| } | ||
|
|
||
| private UUID maybeUuid(String input) { |
There was a problem hiding this comment.
Possibly a place for an Optional instead of a null? (disclosure: I generally steer clear of nulls in Java if possible)
There was a problem hiding this comment.
I'm gonna push back here - I think that's sound advice to use Optional instead of null for public methods, but this is a private method used only within the class. If it returned Optional, it would require additional steps inside of executeInternal(). 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.
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!
...ice/src/main/java/org/databiosphere/workspacedataservice/dataimport/PfbQuartzJobSupport.java
Outdated
Show resolved
Hide resolved
| PfbQuartzJobSupport pfbQuartzJobSupport = | ||
| new PfbQuartzJobSupport(workspaceId, wsmDao, restClientRetry); | ||
| List<UUID> existingSnapshotIds = | ||
| pfbQuartzJobSupport.existingPolicySnapshotIds(/* pageSize= */ 50); |
There was a problem hiding this comment.
Thanks for the arg comment ❤️
...ice/src/main/java/org/databiosphere/workspacedataservice/dataimport/PfbQuartzJobSupport.java
Outdated
Show resolved
Hide resolved
...ice/src/main/java/org/databiosphere/workspacedataservice/dataimport/PfbQuartzJobSupport.java
Outdated
Show resolved
Hide resolved
| } | ||
| } | ||
| } | ||
| return null; |
There was a problem hiding this comment.
Another possible place for an Optional?
There was a problem hiding this comment.
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
| import org.springframework.http.HttpStatus; | ||
| import org.springframework.web.bind.annotation.ResponseStatus; | ||
|
|
||
| @ResponseStatus(code = HttpStatus.INTERNAL_SERVER_ERROR) |
There was a problem hiding this comment.
The way PfbImportException gets thrown (for blowing the hard limit), and the fact that it subclasses IllegalArgumentException 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.
The use of IllegalArgumentException here was a copy/paste mistake on my part, I have changed to extend RuntimeException!
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 a while(true) loop, but I got nervous about infinity and decided to add a sanity check and thus imposed a hard limit.
| } | ||
| } | ||
|
|
||
| throw new PfbImportException( |
There was a problem hiding this comment.
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.
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.
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.
|
Kudos, SonarCloud Quality Gate passed! |
When importing a PFB: