Copilot commented on code in PR #18058:
URL: https://github.com/apache/pinot/pull/18058#discussion_r3031108947
##########
pinot-core/src/test/java/org/apache/pinot/core/transport/QueryRoutingTest.java:
##########
@@ -53,22 +54,18 @@
public class QueryRoutingTest {
- private static final int TEST_PORT = 12345;
- private static final ServerInstance SERVER_INSTANCE = new
ServerInstance("localhost", TEST_PORT);
- private static final ServerRoutingInstance OFFLINE_SERVER_ROUTING_INSTANCE =
- SERVER_INSTANCE.toServerRoutingInstance(TableType.OFFLINE,
ServerInstance.RoutingType.NETTY);
- private static final ServerRoutingInstance REALTIME_SERVER_ROUTING_INSTANCE =
- SERVER_INSTANCE.toServerRoutingInstance(TableType.REALTIME,
ServerInstance.RoutingType.NETTY);
private static final BrokerRequest BROKER_REQUEST =
CalciteSqlCompiler.compileToBrokerRequest("SELECT * FROM testTable");
- private static final Map<ServerInstance, SegmentsToQuery> ROUTING_TABLE =
- Collections.singletonMap(SERVER_INSTANCE,
- new SegmentsToQuery(Collections.emptyList(),
Collections.emptyList()));
private QueryRouter _queryRouter;
private ServerRoutingStatsManager _serverRoutingStatsManager;
int _requestCount;
private QueryServer _queryServer;
+ private int _testPort;
+ private ServerInstance _serverInstance;
+ private ServerRoutingInstance _offlineServerRoutingInstance;
+ private ServerRoutingInstance _realtimeServerRoutingInstance;
+ private Map<ServerInstance, SegmentsToQuery> _routingTable;
Review Comment:
The new field `_testPort` is only used to pass through to `new
ServerInstance("localhost", _testPort)` and is never read elsewhere. This extra
mutable state makes the test harder to follow; consider removing `_testPort`
and using the `port` parameter directly (or returning a small fixture object
from `initializeTestPort`).
##########
pinot-core/src/test/java/org/apache/pinot/core/transport/QueryRoutingTest.java:
##########
@@ -106,6 +124,23 @@ private QueryServer getQueryServer(int responseDelayMs,
byte[] responseBytes, in
return new QueryServer(port, null, handler);
}
+ /**
+ * Starts the query server and waits for it to be ready. Uses port 0 to let
the OS assign a free port,
+ * avoiding TOCTOU race conditions. Returns the actual bound port.
+ */
+ private int startQueryServerWithWait(QueryServer server) throws Exception {
+ server.start();
+ // Wait for server to be fully ready with a timeout
+ TestUtils.waitForCondition(aVoid -> {
+ try {
+ return server.getChannel() != null && server.getChannel().isActive();
+ } catch (Exception e) {
+ return false;
+ }
+ }, 10L, 5000, "Query server failed to start");
+ return ((InetSocketAddress) server.getChannel().localAddress()).getPort();
+ }
Review Comment:
`startQueryServerWithWait()` adds an active-channel polling loop, but
`QueryServer.start()` already blocks on `bind(...).sync()` (see
`QueryServer#start`), so the server should be ready when `start()` returns.
Consider simplifying this helper to just `server.start()` + reading
`localAddress().getPort()`, or switch to
`TestUtils.waitForCondition(SupplierWithException, ...)` if you expect
`getChannel()`/`isActive()` to throw and want visibility into those errors.
##########
pinot-core/src/test/java/org/apache/pinot/core/transport/QueryRoutingTest.java:
##########
@@ -85,18 +82,39 @@ public void setUp() {
@AfterMethod
void shutdownServer() {
if (_queryServer != null) {
- _queryServer.shutDown();
+ try {
+ _queryServer.shutDown();
+ } catch (Exception e) {
+ // Ignore exceptions during shutdown
+ }
_queryServer = null;
}
+ // Wait a bit to ensure the port is released
+ try {
+ Thread.sleep(100);
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ }
Review Comment:
The unconditional `Thread.sleep(100)` in `@AfterMethod` adds fixed latency
to every test run and still doesn’t guarantee the port is released. Since
`QueryServer.shutDown()` already blocks on `channel.close().sync()` and shuts
down the event loops, consider removing this sleep, or replacing it with a
condition-based wait on the channel close/boss+worker termination if there’s a
known race to address.
```suggestion
```
##########
pinot-core/src/test/java/org/apache/pinot/core/transport/QueryRoutingTest.java:
##########
@@ -85,18 +82,39 @@ public void setUp() {
@AfterMethod
void shutdownServer() {
if (_queryServer != null) {
- _queryServer.shutDown();
+ try {
+ _queryServer.shutDown();
+ } catch (Exception e) {
+ // Ignore exceptions during shutdown
+ }
_queryServer = null;
Review Comment:
`shutdownServer()` swallows all exceptions from `_queryServer.shutDown()`.
If shutdown fails, the test may leak Netty resources/ports and cause
harder-to-debug follow-on failures. Prefer failing the test (e.g.,
`Assert.fail(...)`) or at least recording the exception and rethrowing after
cleanup so port-release issues aren’t silently hidden.
##########
pinot-core/src/test/java/org/apache/pinot/core/transport/QueryRoutingTest.java:
##########
@@ -477,9 +499,34 @@ public void testSkipUnavailableServer()
dataTableMetadata.put(MetadataKey.REQUEST_ID.getName(),
Long.toString(requestId));
byte[] successResponseBytes = dataTableSuccess.toBytes();
- // Only start a single QueryServer, on port from serverInstance1
- _queryServer = getQueryServer(500, successResponseBytes, port);
- _queryServer.start();
+ // Start server1 on port 0 (OS-assigned) to avoid TOCTOU race
+ _queryServer = getQueryServer(500, successResponseBytes, 0);
+ int port1 = startQueryServerWithWait(_queryServer);
+
+ // For server2 (unavailable server), use a port that is guaranteed to have
nothing listening.
+ // Use ServerSocket(0) to find a free port, then close it — no TOCTOU risk
here since we intentionally
+ // want nothing bound on this port.
+ int port2;
+ try (ServerSocket socket = new ServerSocket(0)) {
+ port2 = socket.getLocalPort();
+ // Ensure port2 != port1
+ while (port2 == port1) {
+ socket.close();
+ try (ServerSocket s2 = new ServerSocket(0)) {
+ port2 = s2.getLocalPort();
+ }
+ }
+ }
Review Comment:
In `testSkipUnavailableServer`, `port2` is selected by binding `new
ServerSocket(0)` and then closing it, and the comment claims this
“guarantee[s]” nothing is listening and that there’s “no TOCTOU risk”. That
isn’t guaranteed: another process/test can bind the port after it’s released,
reintroducing flakiness. A more deterministic approach would be to use an
intentionally invalid/unreachable endpoint (e.g., port 0 for the unavailable
server) or otherwise structure the test so the failure mode doesn’t depend on a
transiently-free port.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
