Fix file settings service tests #115770
Fix file settings service tests #115770
Conversation
n1v0lg
added
>test
|
@elasticmachine update branch |
| @@ -223,40 +227,40 @@ public void testProcessFileChanges() throws Exception { | |||
| return null; | |||
| }).when(controller).process(any(), any(XContentParser.class), any(), any()); | |||
|
|
|||
| // we get three events: initial clusterChanged event, first write, second write | |||
There was a problem hiding this comment.
This was not a very robust setup... The two things we really care about is that processFileOnServiceStart gets called, and that processFileChanges and that we wait for these calls. I've restructured this test to use two separate latches, and do the file writes one at a time.
…rch into fix-file-settings-test
| Files.writeString(tempFilePath, contents); | ||
| Files.move(tempFilePath, path, StandardCopyOption.ATOMIC_MOVE, StandardCopyOption.REPLACE_EXISTING); | ||
| } | ||
| private static void writeTestFile(Path path, String contents) { |
There was a problem hiding this comment.
Tweaking this to:
- account for missing atomic_move functionality
- make temp file removal more explicit
|
Pinging @elastic/es-core-infra (Team:Core/Infra) |
|
|
||
| // wait for listener to be called (once for initial processing, once for subsequent update) | ||
| assertTrue(latch.await(20, TimeUnit.SECONDS)); | ||
| assertTrue(changesOnStartLatch.await(20, TimeUnit.SECONDS)); |
There was a problem hiding this comment.
nit: Could use the safeAwait here to follow conventions in other tests.
There was a problem hiding this comment.
ah it has 10 seconds baked in. it's obnoxious that we'd need to wait longer than that but a comment in this file makes me hesitant:
"// we need to wait a bit, on MacOS it may take up to 10 seconds for the Java watcher service to notice the file"
server/src/test/java/org/elasticsearch/reservedstate/service/FileSettingsServiceTests.java
Outdated
Show resolved
Hide resolved
n1v0lg
added
the
auto-merge-without-approval
|
@elasticmachine update branch |
💔 Backport failedThe backport operation could not be completed due to the following error: You can use sqren/backport to manually backport by running |
This PR addresses some of the failure causes tracked under elastic#115280 and elastic#115725: the latch-await setup was rather convoluted and the move command not always correctly invoked in the right order. This PR cleans up latching by separating awaiting the first processing call (on start) from waiting on the subsequent call. Also, it makes writing the file more robust w.r.t. OS'es where `atomic_move` may not be available. This should address failures around the timeout await, and the assertion failures around invoked methods tracked here: elastic#115725 (comment) But will likely require another round of changes to address the failures to delete files. Relates: elastic#115280 Relates: elastic#115725
This PR addresses some of the failure causes tracked under #115280 and #115725: the latch-await setup was rather convoluted and the move command not always correctly invoked in the right order. This PR cleans up latching by separating awaiting the first processing call (on start) from waiting on the subsequent call. Also, it makes writing the file more robust w.r.t. OS'es where
atomic_movemay not be available.This should address failures around the timeout await, and the assertion failures around invoked methods tracked here:
#115725 (comment)
But will likely require another round of changes to address the failures to delete files.
Relates: #115280
Relates: #115725