MSQ: Use task context flag useConcurrentLocks to determine task lock type#17193
MSQ: Use task context flag useConcurrentLocks to determine task lock type#17193cryptoe merged 1 commit intoapache:masterfrom
Conversation
github-actions
Bot
added
Area - Batch Ingestion
Area - MSQ
| /** | ||
| * Task lock type. | ||
| */ | ||
| TaskLockType taskLockType(); |
There was a problem hiding this comment.
This seems weird to put it in the context.
From the code, it seems the primary purpose of this is to aid testing ?
There was a problem hiding this comment.
I would suggest removing it. We already have the MSQ controller task for assertion in the UT's.
There was a problem hiding this comment.
From the code, it seems the primary purpose of this is to aid testing ?
It is needed to get the task lock type in ControllerImpl.
Since ControllerImpl doesn't have access to the task context (present in the MSQControllerTask), I had to add this.
Do you think there is a cleaner way to achieve the same?
There was a problem hiding this comment.
Do you think it is better to add a method to just return the task object itself?
MSQControllerTask task();
There was a problem hiding this comment.
That would become a cyclic dependency.
The current approach seems better. Thanks for the explanation.
There was a problem hiding this comment.
This seems weird to me as well. ControllerContext seems like a place to add stuff that is specific for a server type (indexer/middle manager/ standalone server), as opposed to a general configuration that does not change across all the servers (like an infra change on how to communicate with workers, create workers, check if the controller is alive etc versus a config change like query timeout). What's the cyclic dependency if we move this out?
Do you think there is a cleaner way to achieve the same?
WorkerImpl has a reference to the WorkerTask during the construction time, which it uses to have access to the context. Can a similar approach be used for controller as well?
There was a problem hiding this comment.
If WorkerImpl already does it, I guess it would make sense to just pass MSQControllerTask as a constructor arg to ControllerImpl.
I looked at the code, it doesn't seem like there should be any cyclic dependency since controller is initialized in MSQControllerTask.runTask() and is not required on object creation.
|
Thanks for the quick review, @cryptoe ! |
…) (#17074) (#17076) (#17077) (#17193) (#17243) Backport the following patches for a clean backport of Dart changes 1. Add "targetPartitionsPerWorker" setting for MSQ. (#17048) 2. MSQ: Improved worker cancellation. (#17046) 3. Add "includeAllCounters()" to WorkerContext. (#17047) 4. MSQ: Include worker context maps in WorkOrders. (#17076) 5. TableInputSpecSlicer changes to support running on Brokers. (#17074) 6. Fix call to MemoryIntrospector in IndexerControllerContext. (#17066) 7. MSQ: Add QueryKitSpec to encapsulate QueryKit params. (#17077) 8. MSQ: Use task context flag useConcurrentLocks to determine task lock type (#17193)
3 participants
Changes
MSQControllerTaskusing both the task context and the query contexttaskLockType()toControllerContextso thatControllerImplmay use the task lock type that is determined in
MsqControllerTaskMSQControllerTaskTestand add test for lock type