You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by "kfaraz (via GitHub)" <gi...@apache.org> on 2025/03/08 12:00:33 UTC
[PR] Use IndexerMetadataStorageCoordinator on Overlord to mark segments as unused (druid)
kfaraz opened a new pull request, #17785:
URL: https://github.com/apache/druid/pull/17785
### Description
#17653 introduces a cache for segment metadata on the Overlord.
This patch ensures that once segments are marked as unused, those updates make it directly
to the cache without needing to poll those updates from the metadata store.
Since payloads of unused segments are not cached, the update where an unused segment becomes used
still needs to be polled from the metadata store.
### Changes
- Add methods to `IndexerMetadataStorageCoordinator` to mark segments as unused
- Use new methods in `OverlordDataSourcesResource`
- This will allow segment metadata cache to avoid unnecessary fetches from metadata store
- Update tests
<hr>
This PR has:
- [ ] been self-reviewed.
- [ ] using the [concurrency checklist](https://github.com/apache/druid/blob/master/dev/code-review/concurrency.md) (Remove this item if the PR doesn't have any relation to concurrency.)
- [ ] added documentation for new or modified features or behaviors.
- [ ] a release note entry in the PR description.
- [ ] added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
- [ ] added or updated version, license, or notice information in [licenses.yaml](https://github.com/apache/druid/blob/master/dev/license.md)
- [ ] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
- [ ] added unit tests or modified existing tests to cover new code paths, ensuring the threshold for [code coverage](https://github.com/apache/druid/blob/master/dev/code-review/code-coverage.md) is met.
- [ ] added integration tests.
- [ ] been tested in a test Druid cluster.
--
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] Improve performance of segment metadata cache on Overlord (druid)
Posted by "gianm (via GitHub)" <gi...@apache.org>.
gianm commented on code in PR #17785:
URL: https://github.com/apache/druid/pull/17785#discussion_r2015048991
##########
server/src/main/java/org/apache/druid/metadata/segment/cache/SegmentRecord.java:
##########
@@ -0,0 +1,106 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.metadata.segment.cache;
+
+import org.apache.druid.java.util.common.DateTimes;
+import org.apache.druid.java.util.common.logger.Logger;
+import org.apache.druid.timeline.SegmentId;
+import org.joda.time.DateTime;
+
+import javax.annotation.Nullable;
+import java.sql.ResultSet;
+
+/**
+ * Represents a single record in the druid_segments table.
+ */
+class SegmentRecord
+{
+ private static final Logger log = new Logger(SegmentRecord.class);
+
+ private final SegmentId segmentId;
+ private final boolean isUsed;
+ private final DateTime lastUpdatedTime;
+
+ SegmentRecord(SegmentId segmentId, boolean isUsed, DateTime lastUpdatedTime)
+ {
+ this.segmentId = segmentId;
+ this.isUsed = isUsed;
+ this.lastUpdatedTime = lastUpdatedTime;
+ }
+
+ public SegmentId getSegmentId()
+ {
+ return segmentId;
+ }
+
+ public boolean isUsed()
+ {
+ return isUsed;
+ }
+
+ public DateTime getLastUpdatedTime()
+ {
+ return lastUpdatedTime;
+ }
+
+ /**
+ * Creates a SegmentRecord from the given result set.
+ *
+ * @return null if an error occurred while reading the record.
+ */
+ @Nullable
+ static SegmentRecord fromResultSet(ResultSet r)
+ {
+ String serializedId = null;
+ String dataSource = null;
+ try {
+ serializedId = r.getString("id");
+ dataSource = r.getString("dataSource");
+
+ final DateTime lastUpdatedTime = nullSafeDate(r.getString(
+ "used_status_last_updated"));
+
+ final SegmentId segmentId = SegmentId.tryParse(dataSource, serializedId);
+ if (segmentId == null) {
+ log.noStackTrace().error(
+ "Could not parse Segment ID[%s] of datasource[%s]",
+ serializedId, dataSource
+ );
+ return null;
+ } else {
+ return new SegmentRecord(segmentId, true, lastUpdatedTime);
+ }
+ }
+ catch (Exception e) {
+ log.noStackTrace().error(
Review Comment:
> Do you think it would be better to create an alert for each failing segment ID separately? (similar to [`SqlSegmentsMetadataManager`](https://github.com/apache/druid/blob/b640e0ca75ab6a5cb1604715d942d2322cc5619d/server/src/main/java/org/apache/druid/metadata/SqlSegmentsMetadataManager.java#L1058-L1061)
> I felt it would be too noisy, so alerted at total count level instead.
I think the way you did it here is OK. It's good that there is an alert, and IMO an alert with a number of bad segments is ok.
--
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] Improve performance of segment metadata cache on Overlord (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on PR #17785:
URL: https://github.com/apache/druid/pull/17785#issuecomment-2754543938
Verified the changes in this patch on the following clusters:
|. |Test cluster|Staging cluster|
|-|------------|---------------|
|Metadata store|MySQL|MySQL|
|Master node|m5.large (2 vCPUs, 8 GB)|m5.large (2 vCPUs, 8 GB)|
|Used segments|600k|120k|
|Unused segments|180k|1.5M|
|Pending segments|15k|1k|
|Old sync time|2500s|50-150s|
|__New full sync time__|850s|25s|
|__New delta sync time__|150s|5s|
--
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] Improve performance of segment metadata cache on Overlord (druid)
Posted by "gianm (via GitHub)" <gi...@apache.org>.
gianm commented on code in PR #17785:
URL: https://github.com/apache/druid/pull/17785#discussion_r2012764162
##########
indexing-service/src/main/java/org/apache/druid/indexing/overlord/http/OverlordDataSourcesResource.java:
##########
@@ -66,19 +67,22 @@ public class OverlordDataSourcesResource
private static final Logger log = new Logger(OverlordDataSourcesResource.class);
private final SegmentsMetadataManager segmentsMetadataManager;
+ private final IndexerMetadataStorageCoordinator metadataStorageCoordinator;
Review Comment:
I just realized that the `SegmentsMetadataManager` is the Coordinator version of a metadata cache. Do you think we're moving towards getting rid of it completely on the Overlord side of things, in favor of using only `IndexerMetadataStorageCoordinator`?
It looks like after this patch, the `SegmentsMetadataManager` is still used in two places on the OL:
- this class, for `markAsUsedNonOvershadowedSegmentsInInterval`, `markAsUsedNonOvershadowedSegments`, and `markSegmentAsUsed`.
- in `OverlordCompactionScheduler`, for `getSnapshotOfDataSourcesWithAllUsedSegments`. to me it seems like this could already be replaced by `IndexerSQLMetadataStorageCoordinator#retrieveAllUsedSegments`. (although the `SegmentsMetadataManager` version would perform better, since it caches the snapshot, and the `IndexerSQLMetadataStorageCoordinator` version needs to build a timeline. I am not sure how much this matters.)
##########
server/src/main/java/org/apache/druid/metadata/segment/cache/SegmentRecord.java:
##########
@@ -0,0 +1,106 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.metadata.segment.cache;
+
+import org.apache.druid.java.util.common.DateTimes;
+import org.apache.druid.java.util.common.logger.Logger;
+import org.apache.druid.timeline.SegmentId;
+import org.joda.time.DateTime;
+
+import javax.annotation.Nullable;
+import java.sql.ResultSet;
+
+/**
+ * Represents a single record in the druid_segments table.
+ */
+class SegmentRecord
+{
+ private static final Logger log = new Logger(SegmentRecord.class);
+
+ private final SegmentId segmentId;
+ private final boolean isUsed;
+ private final DateTime lastUpdatedTime;
+
+ SegmentRecord(SegmentId segmentId, boolean isUsed, DateTime lastUpdatedTime)
+ {
+ this.segmentId = segmentId;
+ this.isUsed = isUsed;
+ this.lastUpdatedTime = lastUpdatedTime;
+ }
+
+ public SegmentId getSegmentId()
+ {
+ return segmentId;
+ }
+
+ public boolean isUsed()
+ {
+ return isUsed;
+ }
+
+ public DateTime getLastUpdatedTime()
+ {
+ return lastUpdatedTime;
+ }
+
+ /**
+ * Creates a SegmentRecord from the given result set.
+ *
+ * @return null if an error occurred while reading the record.
+ */
+ @Nullable
+ static SegmentRecord fromResultSet(ResultSet r)
+ {
+ String serializedId = null;
+ String dataSource = null;
+ try {
+ serializedId = r.getString("id");
+ dataSource = r.getString("dataSource");
+
+ final DateTime lastUpdatedTime = nullSafeDate(r.getString(
+ "used_status_last_updated"));
+
+ final SegmentId segmentId = SegmentId.tryParse(dataSource, serializedId);
+ if (segmentId == null) {
+ log.noStackTrace().error(
+ "Could not parse Segment ID[%s] of datasource[%s]",
+ serializedId, dataSource
+ );
+ return null;
+ } else {
+ return new SegmentRecord(segmentId, true, lastUpdatedTime);
+ }
+ }
+ catch (Exception e) {
+ log.noStackTrace().error(
Review Comment:
When might this happen? Does it mean we will ignore segments that we can't read? That would be worrying, because `Exception` is very broad. Is there some kind of scenario you have in mind where a failure is possible?
Also, `.noStackTrace().error` is not a combination I expect to see. Typically `error` is reserved for serious problems, and for serious problems we should always have a stack trace.
##########
processing/src/main/java/org/apache/druid/common/utils/IdUtils.java:
##########
@@ -167,4 +169,24 @@ public static SegmentId getValidSegmentId(String dataSource, String serializedSe
return parsedSegmentId;
}
}
+
+ /**
+ * Tries to parse the given serialized ID as {@link SegmentId}s of the given
Review Comment:
This method is suspicious to me, since it encourages ignoring seemingly-invalid segment IDs. Where are the places that we are encountering segment IDs that may not be valid? Is it ok to ignore them?
--
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] Improve performance of segment metadata cache on Overlord (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on code in PR #17785:
URL: https://github.com/apache/druid/pull/17785#discussion_r2013070438
##########
server/src/main/java/org/apache/druid/metadata/segment/cache/SegmentRecord.java:
##########
@@ -0,0 +1,106 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.metadata.segment.cache;
+
+import org.apache.druid.java.util.common.DateTimes;
+import org.apache.druid.java.util.common.logger.Logger;
+import org.apache.druid.timeline.SegmentId;
+import org.joda.time.DateTime;
+
+import javax.annotation.Nullable;
+import java.sql.ResultSet;
+
+/**
+ * Represents a single record in the druid_segments table.
+ */
+class SegmentRecord
+{
+ private static final Logger log = new Logger(SegmentRecord.class);
+
+ private final SegmentId segmentId;
+ private final boolean isUsed;
+ private final DateTime lastUpdatedTime;
+
+ SegmentRecord(SegmentId segmentId, boolean isUsed, DateTime lastUpdatedTime)
+ {
+ this.segmentId = segmentId;
+ this.isUsed = isUsed;
+ this.lastUpdatedTime = lastUpdatedTime;
+ }
+
+ public SegmentId getSegmentId()
+ {
+ return segmentId;
+ }
+
+ public boolean isUsed()
+ {
+ return isUsed;
+ }
+
+ public DateTime getLastUpdatedTime()
+ {
+ return lastUpdatedTime;
+ }
+
+ /**
+ * Creates a SegmentRecord from the given result set.
+ *
+ * @return null if an error occurred while reading the record.
+ */
+ @Nullable
+ static SegmentRecord fromResultSet(ResultSet r)
+ {
+ String serializedId = null;
+ String dataSource = null;
+ try {
+ serializedId = r.getString("id");
+ dataSource = r.getString("dataSource");
+
+ final DateTime lastUpdatedTime = nullSafeDate(r.getString(
+ "used_status_last_updated"));
+
+ final SegmentId segmentId = SegmentId.tryParse(dataSource, serializedId);
+ if (segmentId == null) {
+ log.noStackTrace().error(
+ "Could not parse Segment ID[%s] of datasource[%s]",
+ serializedId, dataSource
+ );
+ return null;
+ } else {
+ return new SegmentRecord(segmentId, true, lastUpdatedTime);
+ }
+ }
+ catch (Exception e) {
+ log.noStackTrace().error(
Review Comment:
> When might this happen? Does it mean we will ignore segments that we can't read? That would be worrying, because Exception is very broad. Is there some kind of scenario you have in mind where a failure is possible?
This can happen in the rare case when the segment payload has been tampered or some other column was not parseable. It is not frequent but it can happen, as I only recently encountered this in a prod DB.
We are not throwing an error here so that the processing can continue with the rest of the segments.
Even though this segment is ignored here, it actually increments a `skippedCount` in the code where this class is used and we raise an alert if `skippedCount > 0`. The log here is just to allow the operator to go through Overlord logs and see which segment IDs failed and why.
Do you think it would be better to create an alert for each failing segment ID separately? (similar to [`SqlSegmentsMetadataManager`](https://github.com/apache/druid/blob/b640e0ca75ab6a5cb1604715d942d2322cc5619d/server/src/main/java/org/apache/druid/metadata/SqlSegmentsMetadataManager.java#L1058-L1061)
I felt it would be too noisy, so alerted at total count level 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
Re: [PR] Improve performance of segment metadata cache on Overlord (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on PR #17785:
URL: https://github.com/apache/druid/pull/17785#issuecomment-2755990050
Thanks for the review, @gianm !
--
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] Improve performance of segment metadata cache on Overlord (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on code in PR #17785:
URL: https://github.com/apache/druid/pull/17785#discussion_r2013658247
##########
server/src/main/java/org/apache/druid/metadata/segment/cache/SegmentRecord.java:
##########
@@ -0,0 +1,106 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.metadata.segment.cache;
+
+import org.apache.druid.java.util.common.DateTimes;
+import org.apache.druid.java.util.common.logger.Logger;
+import org.apache.druid.timeline.SegmentId;
+import org.joda.time.DateTime;
+
+import javax.annotation.Nullable;
+import java.sql.ResultSet;
+
+/**
+ * Represents a single record in the druid_segments table.
+ */
+class SegmentRecord
+{
+ private static final Logger log = new Logger(SegmentRecord.class);
+
+ private final SegmentId segmentId;
+ private final boolean isUsed;
+ private final DateTime lastUpdatedTime;
+
+ SegmentRecord(SegmentId segmentId, boolean isUsed, DateTime lastUpdatedTime)
+ {
+ this.segmentId = segmentId;
+ this.isUsed = isUsed;
+ this.lastUpdatedTime = lastUpdatedTime;
+ }
+
+ public SegmentId getSegmentId()
+ {
+ return segmentId;
+ }
+
+ public boolean isUsed()
+ {
+ return isUsed;
+ }
+
+ public DateTime getLastUpdatedTime()
+ {
+ return lastUpdatedTime;
+ }
+
+ /**
+ * Creates a SegmentRecord from the given result set.
+ *
+ * @return null if an error occurred while reading the record.
+ */
+ @Nullable
+ static SegmentRecord fromResultSet(ResultSet r)
+ {
+ String serializedId = null;
+ String dataSource = null;
+ try {
+ serializedId = r.getString("id");
+ dataSource = r.getString("dataSource");
+
+ final DateTime lastUpdatedTime = nullSafeDate(r.getString(
+ "used_status_last_updated"));
+
+ final SegmentId segmentId = SegmentId.tryParse(dataSource, serializedId);
+ if (segmentId == null) {
+ log.noStackTrace().error(
+ "Could not parse Segment ID[%s] of datasource[%s]",
+ serializedId, dataSource
+ );
+ return null;
+ } else {
+ return new SegmentRecord(segmentId, true, lastUpdatedTime);
+ }
+ }
+ catch (Exception e) {
+ log.noStackTrace().error(
Review Comment:
Updated.
--
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] Improve performance of segment metadata cache on Overlord (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on code in PR #17785:
URL: https://github.com/apache/druid/pull/17785#discussion_r2013070438
##########
server/src/main/java/org/apache/druid/metadata/segment/cache/SegmentRecord.java:
##########
@@ -0,0 +1,106 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.metadata.segment.cache;
+
+import org.apache.druid.java.util.common.DateTimes;
+import org.apache.druid.java.util.common.logger.Logger;
+import org.apache.druid.timeline.SegmentId;
+import org.joda.time.DateTime;
+
+import javax.annotation.Nullable;
+import java.sql.ResultSet;
+
+/**
+ * Represents a single record in the druid_segments table.
+ */
+class SegmentRecord
+{
+ private static final Logger log = new Logger(SegmentRecord.class);
+
+ private final SegmentId segmentId;
+ private final boolean isUsed;
+ private final DateTime lastUpdatedTime;
+
+ SegmentRecord(SegmentId segmentId, boolean isUsed, DateTime lastUpdatedTime)
+ {
+ this.segmentId = segmentId;
+ this.isUsed = isUsed;
+ this.lastUpdatedTime = lastUpdatedTime;
+ }
+
+ public SegmentId getSegmentId()
+ {
+ return segmentId;
+ }
+
+ public boolean isUsed()
+ {
+ return isUsed;
+ }
+
+ public DateTime getLastUpdatedTime()
+ {
+ return lastUpdatedTime;
+ }
+
+ /**
+ * Creates a SegmentRecord from the given result set.
+ *
+ * @return null if an error occurred while reading the record.
+ */
+ @Nullable
+ static SegmentRecord fromResultSet(ResultSet r)
+ {
+ String serializedId = null;
+ String dataSource = null;
+ try {
+ serializedId = r.getString("id");
+ dataSource = r.getString("dataSource");
+
+ final DateTime lastUpdatedTime = nullSafeDate(r.getString(
+ "used_status_last_updated"));
+
+ final SegmentId segmentId = SegmentId.tryParse(dataSource, serializedId);
+ if (segmentId == null) {
+ log.noStackTrace().error(
+ "Could not parse Segment ID[%s] of datasource[%s]",
+ serializedId, dataSource
+ );
+ return null;
+ } else {
+ return new SegmentRecord(segmentId, true, lastUpdatedTime);
+ }
+ }
+ catch (Exception e) {
+ log.noStackTrace().error(
Review Comment:
> When might this happen? Does it mean we will ignore segments that we can't read? That would be worrying, because Exception is very broad. Is there some kind of scenario you have in mind where a failure is possible?
This can happen in the rare case when the segment payload has been tampered or some other column was not parseable. It is not frequent but it can happen, as I only recently encountered this in a prod DB.
We are not throwing an error here so that the processing can continue with the rest of the segments.
Even though this segment is ignored here, it actually increments a `skippedCount` in the code where this class is used and we raise an alert if `skippedCount > 0`.
The log here is just to allow the operator to go through Overlord logs and see which segment IDs failed and why.
--
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] Improve performance of segment metadata cache on Overlord (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on code in PR #17785:
URL: https://github.com/apache/druid/pull/17785#discussion_r2013070581
##########
server/src/main/java/org/apache/druid/metadata/segment/cache/SegmentRecord.java:
##########
@@ -0,0 +1,106 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.metadata.segment.cache;
+
+import org.apache.druid.java.util.common.DateTimes;
+import org.apache.druid.java.util.common.logger.Logger;
+import org.apache.druid.timeline.SegmentId;
+import org.joda.time.DateTime;
+
+import javax.annotation.Nullable;
+import java.sql.ResultSet;
+
+/**
+ * Represents a single record in the druid_segments table.
+ */
+class SegmentRecord
+{
+ private static final Logger log = new Logger(SegmentRecord.class);
+
+ private final SegmentId segmentId;
+ private final boolean isUsed;
+ private final DateTime lastUpdatedTime;
+
+ SegmentRecord(SegmentId segmentId, boolean isUsed, DateTime lastUpdatedTime)
+ {
+ this.segmentId = segmentId;
+ this.isUsed = isUsed;
+ this.lastUpdatedTime = lastUpdatedTime;
+ }
+
+ public SegmentId getSegmentId()
+ {
+ return segmentId;
+ }
+
+ public boolean isUsed()
+ {
+ return isUsed;
+ }
+
+ public DateTime getLastUpdatedTime()
+ {
+ return lastUpdatedTime;
+ }
+
+ /**
+ * Creates a SegmentRecord from the given result set.
+ *
+ * @return null if an error occurred while reading the record.
+ */
+ @Nullable
+ static SegmentRecord fromResultSet(ResultSet r)
+ {
+ String serializedId = null;
+ String dataSource = null;
+ try {
+ serializedId = r.getString("id");
+ dataSource = r.getString("dataSource");
+
+ final DateTime lastUpdatedTime = nullSafeDate(r.getString(
+ "used_status_last_updated"));
+
+ final SegmentId segmentId = SegmentId.tryParse(dataSource, serializedId);
+ if (segmentId == null) {
+ log.noStackTrace().error(
+ "Could not parse Segment ID[%s] of datasource[%s]",
+ serializedId, dataSource
+ );
+ return null;
+ } else {
+ return new SegmentRecord(segmentId, true, lastUpdatedTime);
+ }
+ }
+ catch (Exception e) {
+ log.noStackTrace().error(
Review Comment:
> Also, .noStackTrace().error is not a combination I expect to see. Typically error is reserved for serious problems, and for serious problems we should always have a stack trace.
Yeah, I see your point. I will include the stack trace here.
--
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] Improve performance of segment metadata cache on Overlord (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on PR #17785:
URL: https://github.com/apache/druid/pull/17785#issuecomment-2754790532
@cryptoe , those are two separate clusters and the results are not really comparable between the two since the underlying metadata store is different.
The comparison is to be done between the old sync time and the new sync time.
Delta sync is sync done after the first sync, where all used segment IDs are fetched but only updated payloads are fetched.
--
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] Improve performance of segment metadata cache on Overlord (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on code in PR #17785:
URL: https://github.com/apache/druid/pull/17785#discussion_r2013068022
##########
processing/src/main/java/org/apache/druid/common/utils/IdUtils.java:
##########
@@ -167,4 +169,24 @@ public static SegmentId getValidSegmentId(String dataSource, String serializedSe
return parsedSegmentId;
}
}
+
+ /**
+ * Tries to parse the given serialized ID as {@link SegmentId}s of the given
Review Comment:
Yeah, I guess it would be better to error out on invalid segment IDs.
The only place these can come from are REST APIs.
--
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] Improve performance of segment metadata cache on Overlord (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on code in PR #17785:
URL: https://github.com/apache/druid/pull/17785#discussion_r2013658694
##########
processing/src/main/java/org/apache/druid/common/utils/IdUtils.java:
##########
@@ -167,4 +169,24 @@ public static SegmentId getValidSegmentId(String dataSource, String serializedSe
return parsedSegmentId;
}
}
+
+ /**
+ * Tries to parse the given serialized ID as {@link SegmentId}s of the given
Review Comment:
Updated.
--
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] Update cache on Overlord directly when marking segments as unused (druid)
Posted by "kgyrtkirk (via GitHub)" <gi...@apache.org>.
kgyrtkirk closed pull request #17785: Update cache on Overlord directly when marking segments as unused
URL: https://github.com/apache/druid/pull/17785
--
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] Improve performance of segment metadata cache on Overlord (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on code in PR #17785:
URL: https://github.com/apache/druid/pull/17785#discussion_r2013067197
##########
indexing-service/src/main/java/org/apache/druid/indexing/overlord/http/OverlordDataSourcesResource.java:
##########
@@ -66,19 +67,22 @@ public class OverlordDataSourcesResource
private static final Logger log = new Logger(OverlordDataSourcesResource.class);
private final SegmentsMetadataManager segmentsMetadataManager;
+ private final IndexerMetadataStorageCoordinator metadataStorageCoordinator;
Review Comment:
Yes, @gianm , I plan to get rid of `SqlSegmentsMetadataManager` from the Overlord completely.
Coordinator will continue to use it for the time being, but eventually we might just get rid of it altogether.
- The usage of `SqlSegmentsMetadataManager` by `OverlordCompactionScheduler` does matter for performance because of the timeline and caching reasons as you guessed, but I should be able to resolve this soon.
--
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] Improve performance of segment metadata cache on Overlord (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on code in PR #17785:
URL: https://github.com/apache/druid/pull/17785#discussion_r2013070438
##########
server/src/main/java/org/apache/druid/metadata/segment/cache/SegmentRecord.java:
##########
@@ -0,0 +1,106 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.metadata.segment.cache;
+
+import org.apache.druid.java.util.common.DateTimes;
+import org.apache.druid.java.util.common.logger.Logger;
+import org.apache.druid.timeline.SegmentId;
+import org.joda.time.DateTime;
+
+import javax.annotation.Nullable;
+import java.sql.ResultSet;
+
+/**
+ * Represents a single record in the druid_segments table.
+ */
+class SegmentRecord
+{
+ private static final Logger log = new Logger(SegmentRecord.class);
+
+ private final SegmentId segmentId;
+ private final boolean isUsed;
+ private final DateTime lastUpdatedTime;
+
+ SegmentRecord(SegmentId segmentId, boolean isUsed, DateTime lastUpdatedTime)
+ {
+ this.segmentId = segmentId;
+ this.isUsed = isUsed;
+ this.lastUpdatedTime = lastUpdatedTime;
+ }
+
+ public SegmentId getSegmentId()
+ {
+ return segmentId;
+ }
+
+ public boolean isUsed()
+ {
+ return isUsed;
+ }
+
+ public DateTime getLastUpdatedTime()
+ {
+ return lastUpdatedTime;
+ }
+
+ /**
+ * Creates a SegmentRecord from the given result set.
+ *
+ * @return null if an error occurred while reading the record.
+ */
+ @Nullable
+ static SegmentRecord fromResultSet(ResultSet r)
+ {
+ String serializedId = null;
+ String dataSource = null;
+ try {
+ serializedId = r.getString("id");
+ dataSource = r.getString("dataSource");
+
+ final DateTime lastUpdatedTime = nullSafeDate(r.getString(
+ "used_status_last_updated"));
+
+ final SegmentId segmentId = SegmentId.tryParse(dataSource, serializedId);
+ if (segmentId == null) {
+ log.noStackTrace().error(
+ "Could not parse Segment ID[%s] of datasource[%s]",
+ serializedId, dataSource
+ );
+ return null;
+ } else {
+ return new SegmentRecord(segmentId, true, lastUpdatedTime);
+ }
+ }
+ catch (Exception e) {
+ log.noStackTrace().error(
Review Comment:
> When might this happen? Does it mean we will ignore segments that we can't read? That would be worrying, because Exception is very broad. Is there some kind of scenario you have in mind where a failure is possible?
This can happen in the rare case when the segment payload has been tampered or some other column was not parseable. It is not frequent but it can happen, as I only recently encountered this in a prod DB.
We are not throwing an error here so that the processing can continue with the rest of the segments.
Even though this segment is ignored here, it actually increments a `skippedCount` in the code where this class is used and we raise an alert if `skippedCount > 0`. The log here is just to allow the operator to go through Overlord logs and see which segment IDs failed and why.
Do you think it would be better to create an alert for each failing segment ID separately? (similar to [`SqlSegmentsMetadataManager`](https://github.com/apache/druid/blob/b640e0ca75ab6a5cb1604715d942d2322cc5619d/server/src/main/java/org/apache/druid/metadata/SqlSegmentsMetadataManager.java#L1058-L1061)
--
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] Improve performance of segment metadata cache on Overlord (druid)
Posted by "cryptoe (via GitHub)" <gi...@apache.org>.
cryptoe commented on PR #17785:
URL: https://github.com/apache/druid/pull/17785#issuecomment-2754781495
@kfaraz
It looks like the new full sync time goes from
25 seconds for 120K used segments
to
850 seconds for 600K used segments .
Could you provide some clarifications around the non linear time increase.
Also could you provide some description around what is the delta sync?
--
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] Improve performance of segment metadata cache on Overlord (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz merged PR #17785:
URL: https://github.com/apache/druid/pull/17785
--
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