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

Implement API for sample management #1063

Merged

Conversation

sven1103
Copy link
Contributor

Provides the async service implementation for the sample management related requests.

@sven1103 sven1103 marked this pull request as ready for review March 10, 2025 15:37
@sven1103 sven1103 requested a review from a team as a code owner March 10, 2025 15:37
@sven1103 sven1103 changed the base branch from main to development March 10, 2025 15:38
@sven1103 sven1103 linked an issue Mar 10, 2025 that may be closed by this pull request
1 task
asyncProjectService.getSamplePreviews(projectId.value(), experimentId.value(),
query.getOffset(), query.getLimit(), List.copyOf(sortOrders), filter).collectList()
.doOnError(RequestFailedException.class, this::handleRequestFailed)
.block()).stream();
Copy link
Member

Choose a reason for hiding this comment

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

How about https://projectreactor.io/docs/core/release/api/reactor/core/publisher/Flux.html#toStream-- instead?
Maybe you do not even need to block then (but I don't know this).
https://projectreactor.io/docs/core/release/reference/apdx-operatorChoice.html#which.blocking
Seems to me that a stream is returned (which is expected by vaadin) so the whole collection into a list and streaming afterwards might not be needed.

.getValue();
var result = asyncProjectService
.getSamples(context.projectId().orElseThrow().value(), experiment.experimentId().value())
.collectList().block();
Copy link
Member

Choose a reason for hiding this comment

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

Instead of waiting for all elements to be there you could only wait for the first element as it is your concern whether the list is empty.
https://projectreactor.io/docs/core/release/reference/apdx-operatorChoice.html#which.blocking
https://projectreactor.io/docs/core/release/api/reactor/core/publisher/Flux.html#blockFirst-java.time.Duration-

Copy link
Contributor Author

@sven1103 sven1103 Mar 11, 2025

Choose a reason for hiding this comment

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

i think that cheats the way Flux is working: we don't know how many samples will be send through the Flux. So maybe it is better to use a convenience method in the Service API instead?

Copy link
Member

Choose a reason for hiding this comment

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

Why would this cheat the way flux is working?
The flux publishes elements and instead of waiting for the flux to complete, you can simply wait for the first element. No need for the flux to continue publishing afterwards as you do not want to use the result.

Copy link
Member

@KochTobi KochTobi Mar 11, 2025

Choose a reason for hiding this comment

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

Of course you could think about adding a method Mono to determine whether there are any samples in an experiment. Depending on the way the flux is created this would minimize the work the system does. But I think both are legitimate ways to handle the situation. And both are better than to wait for the flux completion.

KochTobi
KochTobi previously approved these changes Mar 11, 2025
@KochTobi KochTobi self-requested a review March 11, 2025 09:04
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
0.0% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud

Copy link
Member

@KochTobi KochTobi left a comment

Choose a reason for hiding this comment

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

LGTM

@sven1103 sven1103 merged commit 9810423 into development Mar 11, 2025
5 of 6 checks passed
@sven1103 sven1103 deleted the feature/#1062-implemtation-of-sample-management-api branch March 11, 2025 11:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implemtation of sample management API
2 participants