[RHCLOUD-30839] Use Events to trigger aggregations #2699

g-duval · 2024-05-06T11:15:50Z

The new aggregation command is compatible with the legacy way to aggregate data.
We have some feature flags on aggregator and engine to be able to test aggregator first, then engine.
Even it the new way is enable on engine, we will still record data on email_aggragation table, until we validate it entirely on prod

Exec plan details about new queries are available in linked Jira ticket.

codecov-commenter · 2024-05-06T11:28:58Z

Codecov Report

Attention: Patch coverage is 62.96296% with 10 lines in your changes are missing coverage. Please review.

Project coverage is 69.00%. Comparing base (1ae18be) to head (8716616).
Report is 2 commits behind head on master.

Files	Patch %	Lines
.../cloud/notifications/DailyEmailAggregationJob.java	38.46%	5 Missing and 3 partials ⚠️
...t/cloud/notifications/config/AggregatorConfig.java	60.00%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #2699      +/-   ##
============================================
- Coverage     69.02%   69.00%   -0.03%     
- Complexity     1800     1805       +5     
============================================
  Files           385      385              
  Lines          7975     7998      +23     
  Branches        686      689       +3     
============================================
+ Hits           5505     5519      +14     
- Misses         2185     2191       +6     
- Partials        285      288       +3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

g-duval · 2024-07-04T13:26:05Z

engine/src/test/java/com/redhat/cloud/notifications/processors/email/EmailAggregatorTest.java

        result.putAll(emailAggregator.getAggregated(application.getId(), aggregationKey, DAILY, LocalDateTime.now(ZoneOffset.UTC).minusMinutes(1), LocalDateTime.now(ZoneOffset.UTC).plusMinutes(1)));
        return result;
    }

-    @CacheInvalidate(cacheName = "recipients-resolver-results")


@CacheInvalidate will perform an asynchronous cache cleaning, but we need this cleanup to by sychonous in tests

Unless something changed in quarkus-cache, the old code should be synchronous because the method returns void and not Uni<Void>. Anyway, the new code is fine so please don't change anything!

MikelAlejoBR

Nice work! It's nice that we're using the "Event" table directly now...

aggregator/src/main/java/com/redhat/cloud/notifications/DailyEmailAggregationJob.java

aggregator/src/main/java/com/redhat/cloud/notifications/db/AggregationOrgConfigRepository.java

aggregator/src/main/java/com/redhat/cloud/notifications/config/AggregatorConfig.java

engine/src/main/java/com/redhat/cloud/notifications/config/EngineConfig.java

engine/src/main/java/com/redhat/cloud/notifications/processors/email/EmailAggregator.java

aggregator/src/main/java/com/redhat/cloud/notifications/DailyEmailAggregationJob.java

aggregator/src/main/java/com/redhat/cloud/notifications/db/AggregationOrgConfigRepository.java

aggregator/src/test/java/com/redhat/cloud/notifications/DailyEventAggregationJobTest.java

g-duval · 2024-07-19T13:07:15Z

/retest

gwenneg · 2024-09-16T07:31:00Z

@g-duval I'll take a look at this PR this week. Could you please rebase it and fix the conflicts?

aggregator/src/main/java/com/redhat/cloud/notifications/db/EmailAggregationRepository.java

common/src/main/java/com/redhat/cloud/notifications/models/EventAggregationCommand.java

aggregator/src/main/java/com/redhat/cloud/notifications/config/AggregatorConfig.java

engine/src/main/java/com/redhat/cloud/notifications/processors/email/EmailAggregator.java

aggregator/src/main/java/com/redhat/cloud/notifications/db/AggregationOrgConfigRepository.java

aggregator/src/main/java/com/redhat/cloud/notifications/DailyEmailAggregationJob.java

aggregator/src/main/java/com/redhat/cloud/notifications/db/EmailAggregationRepository.java

aggregator/src/test/java/com/redhat/cloud/notifications/DailyEventAggregationJobTest.java

aggregator/src/test/resources/mockito-extensions/org.mockito.plugins.MockMaker

aggregator/src/test/java/com/redhat/cloud/notifications/helpers/ResourceHelpers.java

g-duval · 2024-11-25T09:04:40Z

/retest

g-duval · 2024-11-25T12:47:12Z

/retest

common-unleash/src/main/java/com/redhat/cloud/notifications/unleash/UnleashConfigSource.java

...m/redhat/cloud/notifications/db/repositories/EmailAggregationRepositoryBasedOnEventTest.java

aggregator/src/main/java/com/redhat/cloud/notifications/DailyEmailAggregationJob.java

aggregator/src/main/java/com/redhat/cloud/notifications/db/EmailAggregationRepository.java

…ation command based on events

aggregator/src/main/java/com/redhat/cloud/notifications/db/EmailAggregationRepository.java

engine/src/main/java/com/redhat/cloud/notifications/processors/email/EmailAggregator.java

gwenneg · 2024-11-26T13:36:05Z

engine/src/test/java/com/redhat/cloud/notifications/processors/email/EmailAggregatorTest.java

        result.putAll(emailAggregator.getAggregated(application.getId(), aggregationKey, DAILY, LocalDateTime.now(ZoneOffset.UTC).minusMinutes(1), LocalDateTime.now(ZoneOffset.UTC).plusMinutes(1)));
        return result;
    }

-    @CacheInvalidate(cacheName = "recipients-resolver-results")


Unless something changed in quarkus-cache, the old code should be synchronous because the method returns void and not Uni<Void>. Anyway, the new code is fine so please don't change anything!

…ilAggregationRepository.java Co-authored-by: Gwenneg Lepage <[email protected]>

…/email/EmailAggregator.java Co-authored-by: Gwenneg Lepage <[email protected]>

gwenneg

LGTM 🎉

g-duval force-pushed the RHCLOUD-30839_useEventsToTriggerAggregations branch from 8716616 to 6679ef0 Compare May 10, 2024 13:06

g-duval force-pushed the RHCLOUD-30839_useEventsToTriggerAggregations branch 2 times, most recently from 0bde785 to 74e151e Compare May 31, 2024 12:00

g-duval force-pushed the RHCLOUD-30839_useEventsToTriggerAggregations branch 4 times, most recently from fbbdc92 to 4678417 Compare July 4, 2024 09:06

g-duval commented Jul 4, 2024

View reviewed changes

g-duval force-pushed the RHCLOUD-30839_useEventsToTriggerAggregations branch from b88152b to 83fbff2 Compare July 4, 2024 14:05

g-duval marked this pull request as ready for review July 5, 2024 13:16

g-duval changed the title ~~WIP [RHCLOUD-30839] Use Events to trigger aggregations~~ [RHCLOUD-30839] Use Events to trigger aggregations Jul 5, 2024

MikelAlejoBR self-requested a review July 10, 2024 06:22

MikelAlejoBR approved these changes Jul 16, 2024

View reviewed changes

g-duval force-pushed the RHCLOUD-30839_useEventsToTriggerAggregations branch 2 times, most recently from 75e7ed6 to eba0e36 Compare July 19, 2024 11:42

g-duval force-pushed the RHCLOUD-30839_useEventsToTriggerAggregations branch from eba0e36 to 97d6dd1 Compare September 16, 2024 13:39

gwenneg self-assigned this Sep 23, 2024

g-duval force-pushed the RHCLOUD-30839_useEventsToTriggerAggregations branch 2 times, most recently from 5c356ee to a4f9a13 Compare November 19, 2024 09:15

gwenneg reviewed Nov 20, 2024

View reviewed changes

aggregator/src/main/java/com/redhat/cloud/notifications/db/EmailAggregationRepository.java Outdated Show resolved Hide resolved

gwenneg reviewed Nov 20, 2024

View reviewed changes

engine/src/main/java/com/redhat/cloud/notifications/processors/email/EmailAggregator.java Show resolved Hide resolved

gwenneg reviewed Nov 20, 2024

View reviewed changes

gwenneg reviewed Nov 21, 2024

View reviewed changes

g-duval force-pushed the RHCLOUD-30839_useEventsToTriggerAggregations branch from 65360d6 to 03d24d4 Compare November 21, 2024 09:46

gwenneg reviewed Nov 25, 2024

View reviewed changes

g-duval added 6 commits November 26, 2024 09:12

[RHCLOUD-30839] Use Events to trigger aggregations

dff3f13

Removing IAggregationCommand interface

512fd6a

Updates from review

1867d07

Add consistency check and org_id filtering capacity to use new aggreg…

4cc1f6f

…ation command based on events

Fix

c4df413

Updates from code review

b02dd64

g-duval force-pushed the RHCLOUD-30839_useEventsToTriggerAggregations branch from a020cf1 to b02dd64 Compare November 26, 2024 09:08

gwenneg reviewed Nov 26, 2024

View reviewed changes

g-duval and others added 2 commits November 26, 2024 14:55

Update aggregator/src/main/java/com/redhat/cloud/notifications/db/Ema…

f449a43

…ilAggregationRepository.java Co-authored-by: Gwenneg Lepage <[email protected]>

Update engine/src/main/java/com/redhat/cloud/notifications/processors…

7e03e78

…/email/EmailAggregator.java Co-authored-by: Gwenneg Lepage <[email protected]>

gwenneg approved these changes Nov 26, 2024

View reviewed changes

g-duval merged commit d4eceb6 into RedHatInsights:master Nov 26, 2024
32 of 33 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RHCLOUD-30839] Use Events to trigger aggregations #2699

[RHCLOUD-30839] Use Events to trigger aggregations #2699

g-duval commented May 6, 2024 •

edited

Loading

codecov-commenter commented May 6, 2024

g-duval Jul 4, 2024

gwenneg Nov 26, 2024

MikelAlejoBR left a comment

g-duval commented Jul 19, 2024

gwenneg commented Sep 16, 2024

g-duval commented Nov 25, 2024

g-duval commented Nov 25, 2024

gwenneg Nov 26, 2024

gwenneg left a comment

[RHCLOUD-30839] Use Events to trigger aggregations #2699

[RHCLOUD-30839] Use Events to trigger aggregations #2699

Conversation

g-duval commented May 6, 2024 • edited Loading

codecov-commenter commented May 6, 2024

Codecov Report

g-duval Jul 4, 2024

Choose a reason for hiding this comment

gwenneg Nov 26, 2024

Choose a reason for hiding this comment

MikelAlejoBR left a comment

Choose a reason for hiding this comment

g-duval commented Jul 19, 2024

gwenneg commented Sep 16, 2024

g-duval commented Nov 25, 2024

g-duval commented Nov 25, 2024

gwenneg Nov 26, 2024

Choose a reason for hiding this comment

gwenneg left a comment

Choose a reason for hiding this comment

g-duval commented May 6, 2024 •

edited

Loading