[Tests] Fix copying files for test cluster #124628
Conversation
In case when file with `.attach_pid` in name was stored in distribution and then deleted, the exception could stop copying/linking files without any sign of issue. The files were then missing in the cluster used in the test causing them sometimes to fail (depending on which files haven't been copied).
When using `Files.walk` it is impossible to catch the IOException and continue walking through files conditionally. It has been replaced with FileVisitor implementation to be able to continue if the exception is caused by files left temporarily by JVM but no longer available.
Also simplify exception handling
jozala
added
>non-issue
>test
|
Pinging @elastic/es-delivery (Team:Delivery) |
| @@ -1295,40 +1298,47 @@ private void syncWithCopy(Path sourceRoot, Path destinationRoot) { | |||
|
|
|||
| private void sync(Path sourceRoot, Path destinationRoot, BiConsumer<Path, Path> syncMethod) { | |||
There was a problem hiding this comment.
Could we reuse IOUtils#sync here? Maybe with tweaking the IOUtils#sync method but that looks mostly duplication we could simplify and avoid
There was a problem hiding this comment.
My understanding was that this duplication is on purpose, to keep the old and new test cluster frameworks separate.
ElasticsearchNode is from the deprecated framework and IOUtils is from the new JUnit rule-based framework.
I believe we do not want the dependency between those two sub-projects, right?
One way to solve this is to move the IOUtils to a different subproject and depend on it from both frameworks sub-projects.
The old framework will probably not go away soon, but when it will, then it would make sense to leave the IOUtils in the :test:test-cluster. Because of that I am leaning towards accepting the duplication.
What do you think?
There was a problem hiding this comment.
ah I see now. I missed we use IOUtils in our new framework only. yeah lets keep it then 👍
In case when file with `.attach_pid` in name was stored in distribution and then deleted, the exception could stop copying/linking files without any sign of issue. The files were then missing in the cluster used in the test causing them sometimes to fail (depending on which files haven't been copied). When using `Files.walk` it is impossible to catch the IOException and continue walking through files conditionally. It has been replaced with FileVisitor implementation to be able to continue if the exception is caused by files left temporarily by JVM but no longer available.
In case when file with `.attach_pid` in name was stored in distribution and then deleted, the exception could stop copying/linking files without any sign of issue. The files were then missing in the cluster used in the test causing them sometimes to fail (depending on which files haven't been copied). When using `Files.walk` it is impossible to catch the IOException and continue walking through files conditionally. It has been replaced with FileVisitor implementation to be able to continue if the exception is caused by files left temporarily by JVM but no longer available.
In case when file with
.attach_pidin name was stored in distribution and then deleted, the exception could stop copying/linking files without any sign of issue. The files were then missing in the cluster used in the tests causing failures (sometimes - depending on which files haven't been copied).