Conversation
mhl-b
added
>test
|
Pinging @elastic/es-data-management (Team:Data Management) |
| MiniDFSNNTopology namenodeTopology = new MiniDFSNNTopology().addNameservice(nameservice); | ||
| builder.nnTopology(namenodeTopology); | ||
| } | ||
| portPlaceholder.close(); |
There was a problem hiding this comment.
Are you sure this gets closed on all possible code paths? What if something throws an exception before it gets here?
Also this is still kinda sketchy, it's still possible that something gets the port in between this line and the point where HDFS grabs the port.
Also this is wrapped in a retry loop, see startMinHdfs(), but if something fails after this line then the retry won't have an open portPlaceholder to use. I think we should instead seek a fresh free port on each attempt.
There was a problem hiding this comment.
Acquiring port on retry loop sounds right. But org.elasticsearch.test.fixtures.hdfs.HdfsFixture#getPort() might be called before HDFS setup, so we need either setup HDFS sooner or put a blocker on the port.
There was a problem hiding this comment.
If you are fine with port placeholder, I can fix missing socket closure on the exception.
There was a problem hiding this comment.
Also this is still kinda sketchy
Yeah, it's a step from broken to less broken, but still broken.
There was a problem hiding this comment.
Also, there are only 8 places so far where we create a new fixture, we can manually pick ports.
There was a problem hiding this comment.
But
org.elasticsearch.test.fixtures.hdfs.HdfsFixture#getPort()might be called before HDFS setup
I see, yes that's a problem for the usages in the YAML tests. This is right at the boundary of what one might reasonably achieve with a YAML test, and I don't think these tests particularly matter for other test runners (the main point of YAML tests) so IMO we should address this by migrating them to a ESRestTestCase suite in which we can control the fixture startup order.
If you are fine with port placeholder, I can fix missing socket closure on the exception.
If we allow for retries to acquire a different port, do we need this placeholder to try and reserve the port for longer?
There was a problem hiding this comment.
Let me try to refactor to a ESRestTestCase
There was a problem hiding this comment.
Made progress today, ported half of the YAML tests to the RestTests. Shouldn't take long.
There was a problem hiding this comment.
I'll close this PR in favour of moving YAML to Rest PR #140142
|
closing in favour of #140142 |
3 participants
There is a substantial time gap between finding free port and binding it in HDFS fixture, that can lead to
java.net.BindException: Address already in use. This PR binds free port until HDFS fixture is ready to start. https://gradle-enterprise.elastic.co/s/inqfl34rgjszo/failures#1