[ML] fix updating opened jobs scheduled events (#31651)#32881
[ML] fix updating opened jobs scheduled events (#31651)#32881benwtrent merged 5 commits intoelastic:masterfrom
Conversation
dimitris-athanasiou
left a comment
There was a problem hiding this comment.
Looks good. However, it would be great to add an integration test for this. Take a look at ScheduledEventsIT.testOnlineUpdate. I think we could add a similar test for this particular case.
| assertEquals(params.getDetectorUpdates(), updateBuilder.build().getDetectorUpdates()); | ||
| assertEquals(params.getModelPlotConfig(), updateBuilder.build().getModelPlotConfig()); | ||
|
|
||
| updateBuilder.setGroups(Arrays.asList("bar")); |
There was a problem hiding this comment.
nit: Why not call this earlier where the rest of the update is built? There is no reason to separate the groups from this test's perspective.
dimitris-athanasiou
left a comment
There was a problem hiding this comment.
Thanks, that's a cool test! Left some comments regarding naming.
| /** | ||
| * An open job that later gets added to a calendar, should take the scheduled events into account | ||
| */ | ||
| public void testUpdatingOpenedJob() throws Exception { |
There was a problem hiding this comment.
We now have 2 tests that deal with online updates. The old one is testing online updates with regard to adding events to a calendar. This one tests online updates with regard to adding the job to a group for which there's calendars. How about we rename those tests to reflect that? testOnlineUpdate -> testAddEventsToOpenJob and testUpdatingOpenedJob -> testAddOpenedJobToGroupWithCaldendar are some suggestive names but if you have better ideas go for it.
There was a problem hiding this comment.
The two hardest problems in software development:
- Cache invalidation
- Naming things
:D
| public void testUpdatingOpenedJob() throws Exception { | ||
| TimeValue bucketSpan = TimeValue.timeValueMinutes(30); | ||
| String groupName = "opened-calendar-job-group"; | ||
| Job.Builder job = createJob("scheduled-events-open-update", bucketSpan); |
There was a problem hiding this comment.
Note that in all those integ-tests we try to name the job after the class/test itself. We found in the past that makes it easier to debug a failed test. Could you please adjust according to the final test name?
dimitris-athanasiou
left a comment
There was a problem hiding this comment.
LGTM (assuming we get a green build)
|
retest this please |
|
retest this please |
|
retest this please |
* elastic/master: (46 commits) NETWORKING: Make RemoteClusterConn. Lazy Resolve DNS (#32764) [DOCS] Splits the users API documentation into multiple pages (#32825) [DOCS] Splits the token APIs into separate pages (#32865) [DOCS] Creates redirects for role management APIs page Bypassing failing test PainlessDomainSplitIT#testHRDSplit (#32966) TEST: Mute testRetentionPolicyChangeDuringRecovery [DOCS] Fixes more broken links to role management APIs [Docs] Tweaks and fixes to rollup docs [DOCS] Fixes links to role management APIs [ML][TEST] Fix BasicRenormalizationIT after adding multibucket feature [DOCS] Splits the roles API documentation into multiple pages (#32794) [TEST] Run pre 6.4 nodes in non-FIPS JVMs (#32901) Make Geo Context Mapping Parsing More Strict (#32821) [ML] fix updating opened jobs scheduled events (#31651) (#32881) Scripted metric aggregations: add deprecation warning and system property to control legacy params (#31597) Tests: Fix timezone conversion in DateTimeUnitTests Enable FIPS140LicenseBootstrapCheck (#32903) Fix InternalAutoDateHistogram reproducible failure (#32723) Remove assertion in testDocStats on deletedDocs counter (#32914) HLRC: Move ML request converters into their own class (#32906) ...
* 6.x: (42 commits) [DOCS] Splits the users API documentation into multiple pages (#32825) [DOCS] Splits the token APIs into separate pages (#32865) [DOCS] Creates redirects for role management APIs page Bypassing failing test PainlessDomainSplitIT#testHRDSplit (#32966) TEST: Mute testRetentionPolicyChangeDuringRecovery [DOCS] Fixes more broken links to role management APIs [Docs] Tweaks and fixes to rollup docs [DOCS] Fixes links to role management APIs [ML][TEST] Fix BasicRenormalizationIT after adding multibucket feature [DOCS] Splits the roles API documentation into multiple pages (#32794) [TEST] Run pre 6.4 nodes in non-FIPS JVMs (#32901) Remove assertion in testDocStats on deletedDocs counter (#32914) [ML] fix updating opened jobs scheduled events (#31651) (#32881) Enable FIPS140LicenseBootstrapCheck (#32903) HLRC: Move ML request converters into their own class (#32906) [DOCS] Update getting-started.asciidoc (#29518) Fix allowed value for HighlighterBuilder encoder in javadocs (#32780) [DOCS] Add "remove a tag" script logic as an example (#32556) RFC: Test that example plugins build stand-alone (#32235) Security: remove put privilege API (#32879) ...
Addresses issue #31651
Now, if there is a group update, we alert that the scheduled events for the job should be updated as well.
Closes #31651