Add Support to tag a minion as Drained #17375
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #17375 +/- ##
============================================
+ Coverage 63.25% 63.27% +0.01%
+ Complexity 1475 1474 -1
============================================
Files 3162 3162
Lines 188668 188708 +40
Branches 28869 28880 +11
============================================
+ Hits 119348 119397 +49
+ Misses 60074 60057 -17
- Partials 9246 9254 +8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR implements a drain mode for Pinot Minion instances to enable graceful scale-down operations. The implementation allows operators to prevent new task assignments to specific minions while existing tasks complete, using Helix's tag-based assignment mechanism.
Key Changes:
- Added
DRAINstate support to the instance state API endpoint - Implemented tag switching mechanism (minion_untagged → minion_drained) to prevent new task assignments
- Added comprehensive test coverage for drain operations including edge cases and concurrency scenarios
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| CommonConstants.java | Added DRAINED_MINION_INSTANCE constant for the new drain tag |
| PinotHelixResourceManagerMinionDrainTest.java | Comprehensive unit tests covering basic drain operations, edge cases (multiple tags, empty tags, already drained), and concurrent safety |
| PinotInstanceRestletResourceTest.java | Integration tests verifying API behavior for drain operations including error cases |
| PinotHelixResourceManager.java | Core drain logic with drainMinionInstance() method and helper for tag replacement |
| StateType.java | Extended enum to include DRAIN state |
| PinotInstanceRestletResource.java | API endpoint updates to handle DRAIN state with validation for minion instances only |
...ntroller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java
Outdated
Show resolved
Hide resolved
...ntroller/src/test/java/org/apache/pinot/controller/api/PinotInstanceRestletResourceTest.java
Outdated
Show resolved
Hide resolved
| * @param instanceName Name of the minion instance to drain | ||
| * @return Response indicating success or failure | ||
| */ | ||
| public synchronized PinotResourceManagerResponse drainMinionInstance(String instanceName) { |
There was a problem hiding this 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.
| @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.") |
There was a problem hiding this 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.
| + "preventing new task assignments while allowing existing tasks to complete.") | |
| + "preventing new task assignments while allowing existing tasks to complete. " | |
| + "The drain operation is idempotent and preserves other tags on the instance.") |
| @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, " |
There was a problem hiding this comment.
does it mean if a minion is tagged to something else, then we cannot drain this node?
There was a problem hiding this comment.
In this scenario, yes, because ideally, if the minion is tagged with something else, then there are no tasks that are currently being assigned to it, unless the table config has minionInstanceTag present
There was a problem hiding this comment.
@xiangfu0 noticed that we can custom-tag minions so that specific tasks get executed on those. So replacing all the tags with minion_drained
...ntroller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java
Outdated
Show resolved
Hide resolved
| "DRAIN state only applies to minion instances. Instance '" + instanceName + "' is not a minion.", | ||
| Response.Status.BAD_REQUEST); | ||
| } | ||
| PinotResourceManagerResponse response = _pinotHelixResourceManager.drainMinionInstance(instanceName); |
There was a problem hiding this comment.
if a minion is in drained mode, when the same minion come up again, the tag is still drained, then it won't be able to serve tasks right ?
There was a problem hiding this comment.
Yes, that is correct. In case a minion is in drained mode, you would need to retag them with minion_untagged so that Helix can start assigning tasks. This is only when the minion is drained, but not removed from the controller
krishna-st
force-pushed
the
kk/minion_drain
branch
from
dc70362 to
e532cfa
Compare
| LOGGER.info("Restored pre-drain tags for minion instance: {}", instanceName); | ||
| // Return success immediately for drained minions - no need to wait for partition states | ||
| // since minions don't have partitions like servers do | ||
| return PinotResourceManagerResponse.success("Successfully restored tags for minion instance: " |
There was a problem hiding this comment.
I think we still need to process the code for enableInstance(xxxxxx).
| } | ||
|
|
||
| public PinotResourceManagerResponse enableInstance(String instanceName) { | ||
| // If enabling a drained minion, restore to its previous tags |
There was a problem hiding this comment.
wrap this restore minion tags logic into a method and put it in to the:
private PinotResourceManagerResponse enableInstance(String instanceName, boolean enableInstance, long timeOutMs)
| // Validate that minion is not already drained | ||
| List<String> currentTags = instanceConfig.getTags(); | ||
| if (currentTags != null && currentTags.contains(Helix.DRAINED_MINION_INSTANCE)) { | ||
| throw new UnsupportedOperationException( |
There was a problem hiding this comment.
this exception won't be caught by the external caller in pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotInstanceRestletResource.java#toggleInstanceState()
I feel return is better:
return PinotResourceManagerResponse.failure("Minion instance " + instanceName + " is already drained");
3 participants
Implements a drain mode for Pinot Minion instances, allowing operators to gracefully scale down minions by preventing new task assignments while allowing existing tasks to complete.
PUT /instances/{instanceName}/stateto supportDRAINstate for minion instancesminion_untagged→minion_drained, preventing Helix from assigning new tasks.setInstanceGroupTag())