clintropolis commented on a change in pull request #11457:
URL: https://github.com/apache/druid/pull/11457#discussion_r683726133
##########
File path:
sql/src/main/java/org/apache/druid/sql/calcite/schema/DruidSchema.java
##########
@@ -494,14 +633,17 @@ void removeServerSegment(final DruidServerMetadata
server, final DataSegment seg
private long recomputeIsRealtime(ImmutableSet<DruidServerMetadata> servers)
{
+ if (servers.isEmpty()) {
+ return 0;
+ }
final Optional<DruidServerMetadata> historicalServer = servers
.stream()
- .filter(metadata -> metadata.getType().equals(ServerType.HISTORICAL))
+ .filter(metadata -> metadata.getType().equals(ServerType.HISTORICAL)
+ || metadata.getType().equals(ServerType.BROKER))
Review comment:
This is correct, but currently broker segments are _not_ tracked in
segment metadata, on the assumption that any segment a broker has is also going
to be somewhere on a historical, and if it isn't on any historicals then it
either will be soon or, dropped from the broker soon, and it doesn't look like
that behavior has changed in this PR.
This isn't great to ignore them, but it does save on potentially complicated
logic of not querying segment metadata to ourself and only other brokers (we
would probably want to get that information in a different way locally?).
Anyway, it might be worth adding a comment about this, even though it is
mentioned in a few other places, like addSegment.
##########
File path:
sql/src/main/java/org/apache/druid/sql/calcite/schema/DruidSchema.java
##########
@@ -129,18 +185,14 @@
@GuardedBy("lock")
private final TreeSet<SegmentId> segmentsNeedingRefresh = new
TreeSet<>(SEGMENT_ORDER);
- // Escalator, so we can attach an authentication result to queries we
generate.
- private final Escalator escalator;
-
@GuardedBy("lock")
private boolean refreshImmediately = false;
- @GuardedBy("lock")
- private long lastRefresh = 0L;
- @GuardedBy("lock")
- private long lastFailure = 0L;
+
@GuardedBy("lock")
private boolean isServerViewInitialized = false;
+ private int totalSegments = 0;
Review comment:
At first I was wondering if this be `volatile`, but I don't think it
matters since it is only called whenever sys segments scan is run, and not in a
loop or anything where it would probably matter. Maybe we should add a comment
that it is ok to be neither guarded by, volatile, or a concurrent type?
##########
File path:
sql/src/main/java/org/apache/druid/sql/calcite/schema/DruidSchema.java
##########
@@ -587,61 +742,117 @@ private long
recomputeIsRealtime(ImmutableSet<DruidServerMetadata> servers)
return retVal;
}
+ /**
+ * This method is not thread-safe and must be used only in unit tests.
+ */
@VisibleForTesting
void setAvailableSegmentMetadata(final SegmentId segmentId, final
AvailableSegmentMetadata availableSegmentMetadata)
{
- synchronized (lock) {
- TreeMap<SegmentId, AvailableSegmentMetadata> dataSourceSegments =
segmentMetadataInfo.computeIfAbsent(
- segmentId.getDataSource(),
- x -> new TreeMap<>(SEGMENT_ORDER)
- );
- if (dataSourceSegments.put(segmentId, availableSegmentMetadata) == null)
{
- totalSegments++;
- }
+ final ConcurrentSkipListMap<SegmentId, AvailableSegmentMetadata>
dataSourceSegments = segmentMetadataInfo
+ .computeIfAbsent(
+ segmentId.getDataSource(),
+ k -> new ConcurrentSkipListMap<>(SEGMENT_ORDER)
+ );
+ if (dataSourceSegments.put(segmentId, availableSegmentMetadata) == null) {
+ totalSegments++;
}
}
- protected DruidTable buildDruidTable(final String dataSource)
+ /**
+ * This is a helper method for unit tests to emulate heavy work done with
{@link #lock}.
+ * It must be used only in unit tests.
Review comment:
nit: maybe nice to move all of these 'only for testing' methods to the
end of this file or something to get them out of the way.
--
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]
