Copilot commented on code in PR #17375:
URL: https://github.com/apache/pinot/pull/17375#discussion_r2624016136
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java:
##########
@@ -3624,6 +3624,55 @@ public PinotResourceManagerResponse dropInstance(String
instanceName) {
return PinotResourceManagerResponse.success("Instance " + instanceName + "
dropped");
}
+ /**
+ * Drains a minion instance by preventing new task assignments while
allowing existing tasks to complete.
+ * This is achieved by changing the instance tag from minion_untagged to
minion_drained, which prevents
+ * Helix from assigning new tasks to this instance.
+ *
+ * @param instanceName Name of the minion instance to drain
+ * @return Response indicating success or failure
+ */
+ public synchronized PinotResourceManagerResponse drainMinionInstance(String
instanceName) {
Review Comment:
The `synchronized` method-level lock could become a bottleneck under
concurrent drain requests across different minion instances. Consider using
instance-level locking or a more granular synchronization mechanism (e.g.,
`ConcurrentHashMap` with per-instance locks) to allow concurrent draining of
different minions.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotInstanceRestletResource.java:
##########
@@ -252,17 +253,20 @@ public SuccessResponse addInstance(
@Authenticate(AccessType.UPDATE)
@Produces(MediaType.APPLICATION_JSON)
@Consumes(MediaType.TEXT_PLAIN)
- @ApiOperation(value = "Enable/disable an instance", notes = "Enable/disable
an instance")
+ @ApiOperation(value = "Enable/disable/drain an instance", notes =
"Enable/disable/drain an instance. "
+ + "DRAIN state is only applicable to minion instances and retags them
from minion_untagged to minion_drained, "
+ + "preventing new task assignments while allowing existing tasks to
complete.")
Review Comment:
The documentation should mention that the drain operation is idempotent and
preserves other tags on the instance. This is important operational context for
users of the API.
```suggestion
+ "preventing new task assignments while allowing existing tasks to
complete. "
+ "The drain operation is idempotent and preserves other tags on the
instance.")
```
--
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]
