You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by "satwik-codeium (via GitHub)" <gi...@apache.org> on 2024/11/13 07:28:49 UTC
[PR] Codeium's Cascade just did this!! (druid)
satwik-codeium opened a new pull request, #17470:
URL: https://github.com/apache/druid/pull/17470
### Description
This PR updates the `SegmentLoadStatusFetcher` to use the new SQL package's `BrokerClient` implementation, replacing the deprecated discovery-based `BrokerClient`. This change improves code maintainability and aligns with Druid's ongoing modernization efforts.
#### Migrated BrokerClient Implementation
- Replaced deprecated `org.apache.druid.discovery.BrokerClient` with `org.apache.druid.sql.client.BrokerClient`
- Updated SQL task handling to use proper task state management through `SqlTaskStatus`
- Fixed locale-dependent string formatting by using `StringUtils.format`
#### Updated Test Framework
- Refactored `SegmentLoadStatusFetcherTest` to properly mock SQL task lifecycle
- Added proper task state transitions (RUNNING → SUCCESS) in test cases
- Maintained existing test coverage while adapting to new API
#### Release note
Migrated Multi-Stage Query's segment load status checking to use the new SQL package's BrokerClient, removing dependency on the deprecated discovery-based implementation. This change improves code maintainability and provides better SQL task state handling.
<hr>
##### Key changed/added classes in this PR
* `org.apache.druid.msq.exec.SegmentLoadStatusFetcher`
* `org.apache.druid.msq.exec.SegmentLoadStatusFetcherTest`
<hr>
This PR has:
- [x] been self-reviewed.
- [x] added documentation for modified behaviors.
- [x] a release note entry in the PR description.
- [x] added comments explaining the "why" and intent of code changes.
- [x] modified existing tests to cover new code paths.
- [x] been tested through unit tests.
Design Decisions:
1. Task State Management:
- Switched to using `TaskState` for explicit state tracking
- Simplified error handling by properly propagating task status
- Improved readability by separating state management from query execution
2. Test Structure:
- Mock responses now simulate realistic task state transitions
- Maintained existing test scenarios while adapting to new API
- Added proper verification of task state changes
3. Error Handling:
- Improved error propagation through `SqlTaskStatus`
- Added explicit handling of failed tasks
- Enhanced logging for better debugging
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Removed org.apache.druid.discovery.BrokerClient by switching to org.apache.druid.sql.client.BrokerClient. Also upgraded SegmentLoadStatusFetcherTest to reflect changes. (druid)
Posted by "cryptoe (via GitHub)" <gi...@apache.org>.
cryptoe closed pull request #17470: Removed org.apache.druid.discovery.BrokerClient by switching to org.apache.druid.sql.client.BrokerClient. Also upgraded SegmentLoadStatusFetcherTest to reflect changes.
URL: https://github.com/apache/druid/pull/17470
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Removed org.apache.druid.discovery.BrokerClient by switching to org.apache.druid.sql.client.BrokerClient. Also upgraded SegmentLoadStatusFetcherTest to reflect changes. (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on code in PR #17470:
URL: https://github.com/apache/druid/pull/17470#discussion_r1976534742
##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/exec/SegmentLoadStatusFetcher.java:
##########
@@ -149,46 +148,46 @@ public void waitForSegmentsToLoad()
try {
FutureUtils.getUnchecked(executorService.submit(() -> {
long lastLogMillis = -TimeUnit.MINUTES.toMillis(1);
- try {
- while (!(hasAnySegmentBeenLoaded.get() && versionLoadStatusReference.get().isLoadingComplete())) {
- // Check the timeout and exit if exceeded.
- long runningMillis = new Interval(startTime, DateTimes.nowUtc()).toDurationMillis();
- if (runningMillis > TIMEOUT_DURATION_MILLIS) {
- log.warn(
- "Runtime[%d] exceeded timeout[%d] while waiting for segments to load. Exiting.",
- runningMillis,
- TIMEOUT_DURATION_MILLIS
- );
- updateStatus(State.TIMED_OUT, startTime);
- return;
- }
+ while (true) {
+ if (DateTimes.nowUtc().getMillis() - startTime.getMillis() > TIMEOUT_DURATION_MILLIS) {
+ log.warn("Timed out waiting for segments to load");
+ break;
+ }
- if (runningMillis - lastLogMillis >= TimeUnit.MINUTES.toMillis(1)) {
- lastLogMillis = runningMillis;
- log.info(
- "Fetching segment load status for datasource[%s] from broker",
- datasource
- );
+ try {
+ SqlQuery sqlQuery = new SqlQuery(
+ StringUtils.format(LOAD_QUERY, datasource, versionsConditionString),
+ ResultFormat.ARRAY,
+ false,
+ false,
+ false,
+ null,
+ null
+ );
+
+ SqlTaskStatus taskStatus = FutureUtils.getUnchecked(brokerClient.submitSqlTask(sqlQuery), true);
+ if (taskStatus.getState() == TaskState.SUCCESS) {
Review Comment:
It is very unlikely that this `taskStatus` will ever be success or even failed. The task would have just been submitted to the Overlord.
Moreover, this use case doesn't require submitting an MSQ task to the Overlord at all since the LOAD_QUERY is just a sys query.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Removed org.apache.druid.discovery.BrokerClient by switching to org.apache.druid.sql.client.BrokerClient. Also upgraded SegmentLoadStatusFetcherTest to reflect changes. (druid)
Posted by "cryptoe (via GitHub)" <gi...@apache.org>.
cryptoe commented on PR #17470:
URL: https://github.com/apache/druid/pull/17470#issuecomment-2702832655
I think this PR is not tested on a cluster at all. We donot support sys queries in MSQ. I am going to close this PR. Please open a new one and test it on a cluster before sending in the patch.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Removed org.apache.druid.discovery.BrokerClient by switching to org.apache.druid.sql.client.BrokerClient. Also upgraded SegmentLoadStatusFetcherTest to reflect changes. (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on code in PR #17470:
URL: https://github.com/apache/druid/pull/17470#discussion_r1976533905
##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/exec/SegmentLoadStatusFetcher.java:
##########
@@ -423,11 +424,15 @@ public enum State
* The time spent waiting for segments to load exceeded org.apache.druid.msq.exec.SegmentLoadWaiter#TIMEOUT_DURATION_MILLIS.
* The SegmentLoadWaiter exited without failing the task.
*/
- TIMED_OUT;
+ TIMED_OUT,
+ /**
+ * All segments which need to be loaded have been loaded, and the SegmentLoadWaiter exited successfully.
+ */
+ DONE;
Review Comment:
how is this different from SUCCESS?
##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/exec/SegmentLoadStatusFetcher.java:
##########
@@ -149,46 +148,46 @@ public void waitForSegmentsToLoad()
try {
FutureUtils.getUnchecked(executorService.submit(() -> {
long lastLogMillis = -TimeUnit.MINUTES.toMillis(1);
- try {
- while (!(hasAnySegmentBeenLoaded.get() && versionLoadStatusReference.get().isLoadingComplete())) {
- // Check the timeout and exit if exceeded.
- long runningMillis = new Interval(startTime, DateTimes.nowUtc()).toDurationMillis();
- if (runningMillis > TIMEOUT_DURATION_MILLIS) {
- log.warn(
- "Runtime[%d] exceeded timeout[%d] while waiting for segments to load. Exiting.",
- runningMillis,
- TIMEOUT_DURATION_MILLIS
- );
- updateStatus(State.TIMED_OUT, startTime);
- return;
- }
+ while (true) {
+ if (DateTimes.nowUtc().getMillis() - startTime.getMillis() > TIMEOUT_DURATION_MILLIS) {
+ log.warn("Timed out waiting for segments to load");
+ break;
+ }
- if (runningMillis - lastLogMillis >= TimeUnit.MINUTES.toMillis(1)) {
- lastLogMillis = runningMillis;
- log.info(
- "Fetching segment load status for datasource[%s] from broker",
- datasource
- );
+ try {
+ SqlQuery sqlQuery = new SqlQuery(
+ StringUtils.format(LOAD_QUERY, datasource, versionsConditionString),
+ ResultFormat.ARRAY,
+ false,
+ false,
+ false,
+ null,
+ null
+ );
+
+ SqlTaskStatus taskStatus = FutureUtils.getUnchecked(brokerClient.submitSqlTask(sqlQuery), true);
+ if (taskStatus.getState() == TaskState.SUCCESS) {
Review Comment:
It is very unlikely that this `taskStatus` will ever be success or even failed. The task would have just been submitted to the Overlord.
Moreover, this use case doesn't require submitting an MSQ task to the Overlord since the LOAD_QUERY is just a sys query.
##########
extensions-core/multi-stage-query/pom.xml:
##########
@@ -60,6 +60,13 @@
<version>${project.parent.version}</version>
<scope>provided</scope>
</dependency>
+ <dependency>
+ <groupId>org.apache.druid</groupId>
+ <artifactId>druid-sql</artifactId>
+ <version>${project.parent.version}</version>
+ <classifier>tests</classifier>
+ <scope>test</scope>
Review Comment:
It is not just used in tests anymore, so `scope` should be `provided` instead.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org