capistrant commented on code in PR #19682:
URL: https://github.com/apache/druid/pull/19682#discussion_r3579659069
##########
server/src/main/java/org/apache/druid/server/SegmentManager.java:
##########
@@ -298,19 +306,27 @@ public void loadSegmentOnBootstrap(
*
* @param dataSegment segment to load
*
+ * @return the input {@code dataSegment}, or a
+ * {@link org.apache.druid.client.DataSegmentAndLoadProfile}
wrapping it when the historical actually
+ * materialized a partial-load footprint. Callers pass the returned
value to the announcement layer so
+ * partial-load announcements carry accurate {@code loadedBytes}.
* @throws SegmentLoadingException if the segment cannot be loaded
* @throws IOException if the segment info cannot be cached on disk
*/
- public void loadSegment(final DataSegment dataSegment) throws
SegmentLoadingException, IOException
+ public DataSegment loadSegment(final DataSegment dataSegment) throws
SegmentLoadingException, IOException
Review Comment:
+1
##########
server/src/main/java/org/apache/druid/segment/loading/PartialSegmentMetadataCacheEntry.java:
##########
@@ -620,6 +620,31 @@ public long getSize()
}
}
+ /**
+ * The rule-declared on-disk footprint for this segment: this metadata
entry's own reservation (V10 header plus
+ * post-mount adjustment) plus the sum of every rule-held bundle's
reservation. Deliberately excludes bundles that
+ * happen to be linked for other reasons (e.g. load on demand). Used by the
historical to stamp accurate
+ * {@code loadedBytes} on partial-load announcements, so inventory-view
accounting reflects what the rule was
+ * configured to pin rather than incidental cache residency.
+ */
+ public long getRealizedBytes()
+ {
+ // Read sizes straight from ruleBundleHolds under entryLock: each hold
owns a final reference to its bundle entry
+ // and PartialSegmentBundleCacheEntry.getSize() reads a final long, no
lock nesting, no null case, no reliance
+ // on external segment-lock ordering.
+ entryLock.lock();
+ try {
+ long total = currentSize;
+ for (StorageLocation.ReservationHold<PartialSegmentBundleCacheEntry>
hold : ruleBundleHolds.values()) {
Review Comment:
hmmm. I think this sounds legit. couple of claude driven UTs for
`SegmentLocalCacheManagerPartialRuleLoadTest` that could help validate any fix
```JAVA
@Test
void testRealizedBytesIncludesPinnedBaseDependency() throws Exception
{
manager = makeManager(true, true);
final StorageLocation location = manager.getLocations().get(0);
manager.load(partialWrapperSegment(List.of(AGG_BUNDLE)));
final PartialSegmentMetadataCacheEntry metadata =
weakReservedMetadata(location, SEGMENT_ID);
final long metadataBytes = metadata.getSize();
final long aggBytes = bundleEntry(location, AGG_BUNDLE).getSize();
final long baseBytes = bundleEntry(location,
Projections.BASE_TABLE_PROJECTION_NAME).getSize();
Assertions.assertTrue(baseBytes > 0, "base dependency must occupy real
on-disk bytes");
final long realized = metadata.getRealizedBytes();
Assertions.assertEquals(
metadataBytes + aggBytes + baseBytes,
realized,
String.format(
"realizedBytes must include the pinned __base dependency:
metadata=%d + selected=%d + base=%d = %d, "
+ "but got %d (short by %d, exactly the base footprint)",
metadataBytes, aggBytes, baseBytes, metadataBytes + aggBytes +
baseBytes,
realized, (metadataBytes + aggBytes + baseBytes) - realized
)
);
}
@Test
void testRealizedBytesCountsSharedBaseDependencyOnce() throws Exception
{
// Two projections in a single rule both pin the same __base. The base
bytes must be counted exactly once.
manager = makeManager(true, true);
final StorageLocation location = manager.getLocations().get(0);
manager.load(partialWrapperSegment(List.of(AGG_BUNDLE,
OTHER_AGG_BUNDLE)));
final PartialSegmentMetadataCacheEntry metadata =
weakReservedMetadata(location, SEGMENT_ID);
final long metadataBytes = metadata.getSize();
final long aggBytes = bundleEntry(location, AGG_BUNDLE).getSize();
final long otherBytes = bundleEntry(location,
OTHER_AGG_BUNDLE).getSize();
final long baseBytes = bundleEntry(location,
Projections.BASE_TABLE_PROJECTION_NAME).getSize();
Assertions.assertEquals(
metadataBytes + aggBytes + otherBytes + baseBytes,
metadata.getRealizedBytes(),
"realizedBytes must include the shared __base exactly once across
both dependent projections"
);
}
private static CacheEntry bundleEntry(StorageLocation location, String
bundleName)
{
final CacheEntry entry =
location.getCacheEntry(new
PartialSegmentBundleCacheEntryIdentifier(SEGMENT_ID, bundleName));
Assertions.assertNotNull(entry, "bundle[" + bundleName + "] must be
resident after load");
return entry;
}
```
--
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]
