fix(storage): fix flaky storage integration tests#13256
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the TestBench utility to use gthread workers and introduces dynamic port allocation for HTTP and gRPC services to improve test isolation. It also appends a unique UUID to container names. Review feedback identifies that the setReuseAddress call in the new findFreePort method is currently ineffective because it is called after the socket is bound; a code suggestion was provided to fix this. Additionally, it is recommended to refactor the Builder constructor to delegate to the existing parameterized constructor to reduce code duplication.
| private static int findFreePort() { | ||
| try (java.net.ServerSocket socket = new java.net.ServerSocket(0)) { | ||
| socket.setReuseAddress(true); | ||
| return socket.getLocalPort(); | ||
| } catch (java.io.IOException e) { | ||
| throw new RuntimeException("Failed to find a free port", e); | ||
| } | ||
| } |
There was a problem hiding this comment.
The setReuseAddress(true) call is ineffective because it is invoked after the ServerSocket has already been bound by the constructor. To be effective, it must be called on an unbound socket before the bind() operation. Additionally, binding specifically to 127.0.0.1 is generally preferred for local port discovery in tests to avoid potential conflicts or security prompts related to external network interfaces.
| private static int findFreePort() { | |
| try (java.net.ServerSocket socket = new java.net.ServerSocket(0)) { | |
| socket.setReuseAddress(true); | |
| return socket.getLocalPort(); | |
| } catch (java.io.IOException e) { | |
| throw new RuntimeException("Failed to find a free port", e); | |
| } | |
| } | |
| private static int findFreePort() { | |
| try (java.net.ServerSocket socket = new java.net.ServerSocket()) { | |
| socket.setReuseAddress(true); | |
| socket.bind(new java.net.InetSocketAddress("127.0.0.1", 0)); | |
| return socket.getLocalPort(); | |
| } catch (java.io.IOException e) { | |
| throw new RuntimeException("Failed to find a free port", e); | |
| } | |
| } |
| private Builder() { | ||
| this( | ||
| false, | ||
| DEFAULT_BASE_URI, | ||
| DEFAULT_GRPC_BASE_URI, | ||
| DEFAULT_IMAGE_NAME, | ||
| DEFAULT_IMAGE_TAG, | ||
| DEFAULT_CONTAINER_NAME); | ||
| int httpPort = findFreePort(); | ||
| int grpcPort = findFreePort(); | ||
| String uuid = java.util.UUID.randomUUID().toString().substring(0, 8); | ||
|
|
||
| this.ignorePullError = false; | ||
| this.baseUri = "http://127.0.0.1:" + httpPort; | ||
| this.gRPCBaseUri = "http://127.0.0.1:" + grpcPort; | ||
| this.dockerImageName = DEFAULT_IMAGE_NAME; | ||
| this.dockerImageTag = DEFAULT_IMAGE_TAG; | ||
| this.containerName = DEFAULT_CONTAINER_NAME + "_" + uuid; | ||
| } |
There was a problem hiding this comment.
This constructor can be simplified by delegating to the existing parameterized constructor. This avoids duplicating field assignments and maintains consistency with the previous implementation style.
private Builder() {
this(
false,
"http://127.0.0.1:" + findFreePort(),
"http://127.0.0.1:" + findFreePort(),
DEFAULT_IMAGE_NAME,
DEFAULT_IMAGE_TAG,
DEFAULT_CONTAINER_NAME + "_" + java.util.UUID.randomUUID().toString().substring(0, 8));
}References
- Reuse pre-configured objects or existing constructors directly instead of duplicating assignments to maintain consistency and reduce bloat.
1 participant
No description provided.