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

moved action filtering to the database query level #12

Conversation

a-sayyed
Copy link

Signed-off-by: Ahmed Sayed [email protected]

Signed-off-by: Ahmed Sayed <[email protected]>
@a-sayyed a-sayyed requested a review from stefbehl May 31, 2019 10:54
@a-sayyed
Copy link
Author

@stefbehl could you please verify the changes done specially in the tests (semantics point of view)? I removed the events checking with EventHandlerStub and replaced it with @ExpectedEvents because they were problematic.

* @return <code>true</code> if one or more active actions for the given
* controllerId and action status are found
*/
boolean existsByTargetControllerIdAndStatusAndActiveIsTrue(@Param("controllerId") String controllerId,
Copy link

Choose a reason for hiding this comment

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

I think you can remove the Param annotations as you do not specify a custom query which references the params

Copy link

@stefbehl stefbehl left a comment

Choose a reason for hiding this comment

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

Thanks for contributing this - looks good!
There is one finding you might wanna take a look at (see my comment).

@@ -1178,73 +1118,6 @@ public DeploymentResult(final Iterable<Target> deployedTs, final Iterable<Target

}

protected static class EventHandlerStub implements ApplicationListener<TargetAssignDistributionSetEvent> {
Copy link
Author

@a-sayyed a-sayyed Jun 12, 2019

Choose a reason for hiding this comment

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

@peerreviewer double check if the eventHandleStub is really not needed (rudimentary).

Copy link
Author

@a-sayyed a-sayyed Jun 12, 2019

Choose a reason for hiding this comment

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

@peerreviewer are the timeouts respected with these changes?

final DistributionSet distributionSet2 = testdataFactory.createDistributionSet(UUID.randomUUID().toString());
registerTargetAndAssignDistributionSet(distributionSet2.getId(), TargetUpdateStatus.PENDING,
getDistributionSet().getModules(), controllerId);
testdataFactory.addSoftwareModuleMetadata(distributionSet2);
Copy link
Author

@a-sayyed a-sayyed Jun 12, 2019

Choose a reason for hiding this comment

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

@peerreviewer double check if the change doesn't change the test behaviour.

@@ -34,7 +34,7 @@
@JsonProperty
private List<DmfArtifact> artifacts;
@JsonProperty
private List<DmfMetadata> metadata;
private List<DmfMetadata> metadata = Collections.emptyList();
Copy link
Author

Choose a reason for hiding this comment

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

revert

@a-sayyed a-sayyed requested a review from bogdan-bondar June 20, 2019 09:40
@stefbehl stefbehl requested review from stefbehl and removed request for bogdan-bondar June 21, 2019 12:04
Copy link

@stefbehl stefbehl left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks!

@stefbehl stefbehl merged commit 2bb0f3d into feature_multi_assignments_optimizations Jun 21, 2019
@stefbehl stefbehl deleted the feature_optimize_query_hasPendingCancellations branch June 21, 2019 12:05
stefbehl added a commit that referenced this pull request Jul 19, 2019
* Add remote event test for the new MultiActionEvent
* Improve test descriptions
* Improve sendMultiActionRequestMessages
* Moved action filtering to the database query level (#12)
* Use @ExpectedEvents instead of EventHandlerStubs
* Removed @param from 'existsByTargetControllerIdAndStatusAndActiveIsTrue'
* Reverted metadata initialization
* Fix hawkBit bot findings

Signed-off-by: Stefan Behl <[email protected]>
Signed-off-by: Ahmed Sayed <[email protected]>
AmmarBikic pushed a commit that referenced this pull request Oct 2, 2020
* Add remote event test for the new MultiActionEvent
* Improve test descriptions
* Improve sendMultiActionRequestMessages
* Moved action filtering to the database query level (#12)
* Use @ExpectedEvents instead of EventHandlerStubs
* Removed @param from 'existsByTargetControllerIdAndStatusAndActiveIsTrue'
* Reverted metadata initialization
* Fix hawkBit bot findings

Signed-off-by: Stefan Behl <[email protected]>
Signed-off-by: Ahmed Sayed <[email protected]>
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