FrankChen021 commented on code in PR #19496:
URL: https://github.com/apache/druid/pull/19496#discussion_r3281508373
##########
processing/src/main/java/org/apache/druid/segment/file/SegmentFileContainerMetadata.java:
##########
@@ -30,30 +31,29 @@
* Starting offset and size of a 'container' stored in a V10 segment file;
think the V10 equivalent of V9's external
* 'smoosh' files, e.g. 00000.smoosh.
* <p>
- * Each container holds internal files belonging to at most one named file
group, as declared at write time via
- * {@link SegmentFileBuilder#startFileGroup}. The {@link #fileGroup} field
records that name so readers can attribute
- * a container to its group without parsing internal-file names. The field is
{@code null} for containers written
- * without a {@code startFileGroup} call (or with {@code
startFileGroup(null)}), and for containers from segments
- * produced by writers that pre-date this field; null serializes as a
Jackson-omitted property so old segments
- * round-trip unchanged.
+ * Each container holds internal files belonging to exactly one named bundle,
as declared at write time via
+ * {@link SegmentFileBuilder#startFileBundle}. The {@link #bundle} field
records that name so readers can attribute a
+ * container to its bundle without parsing internal-file names. Containers
written without an explicit
+ * {@code startFileBundle} call are tagged with {@link
SegmentFileBuilder#ROOT_BUNDLE_NAME}; that default value is
+ * omitted from JSON output, so segments produced by writers pre-dating this
field deserialize cleanly (missing
+ * property normalizes to the default in the constructor).
*/
public class SegmentFileContainerMetadata
{
private final long startOffset;
private final long size;
- @Nullable
- private final String fileGroup;
+ private final String bundle;
@JsonCreator
public SegmentFileContainerMetadata(
@JsonProperty("startOffset") long startOffset,
@JsonProperty("size") long size,
- @JsonProperty("fileGroup") @Nullable String fileGroup
+ @JsonProperty("bundle") @Nullable String bundle
Review Comment:
[P1] Preserve old fileGroup metadata on read
This constructor now only binds the new `bundle` JSON property, so V10
metadata already written by current master with `"fileGroup":"projA"` is read
as null and normalized to `ROOT_BUNDLE_NAME`. After upgrade, grouped containers
from existing segments are no longer discoverable as their original
projection/base bundle, so partial bootstrap/acquire paths will either restore
the wrong root bundle or fail to find the requested bundle. Please accept both
names, for example with a `fileGroup` alias/backcompat creator path, while
writing only `bundle` if desired.
##########
server/src/main/java/org/apache/druid/segment/loading/StorageLocation.java:
##########
@@ -423,6 +423,71 @@ public <T extends CacheEntry> ReservationHold<T>
addWeakReservationHold(
}
}
+ /**
+ * Adjusts the reservation size of an already-registered {@link
ResizableCacheEntry} downward. Used when an entry's
+ * final size is not known at registration time (e.g. a partial-segment
metadata entry that reserves a pessimistic
+ * estimate and shrinks to the actual on-disk header size once the header
has been downloaded). Returns reclaimed
+ * capacity to the location's available budget; never triggers eviction.
+ * <p>
+ * Throws if {@code newSize} is greater than the entry's current size: grow
semantics require checking the location's
+ * available budget and possibly evicting other entries, and aren't needed
by the current callers.
+ */
+ public void adjustReservation(CacheEntryIdentifier id, long newSize)
+ {
+ lock.writeLock().lock();
+ try {
+ final CacheEntry entry;
+ final boolean isStatic;
+ if (staticCacheEntries.containsKey(id)) {
+ entry = staticCacheEntries.get(id);
+ isStatic = true;
+ } else {
+ final WeakCacheEntry weak = weakCacheEntries.get(id);
+ if (weak == null) {
+ throw DruidException.defensive(
+ "Cannot adjust reservation for unknown cache entry[%s]",
+ id
+ );
+ }
+ entry = weak.cacheEntry;
+ isStatic = false;
+ }
+
+ if (!(entry instanceof ResizableCacheEntry)) {
+ throw DruidException.defensive(
+ "Cache entry[%s] of type[%s] does not support reservation
adjustment",
+ id,
+ entry.getClass().getSimpleName()
+ );
+ }
+
+ final long oldSize = entry.getSize();
+ final long delta = oldSize - newSize;
+ if (delta < 0) {
+ throw DruidException.defensive(
+ "Cannot grow reservation for cache entry[%s] from [%d] to [%d]
bytes; only shrink is supported",
+ id,
+ oldSize,
+ newSize
+ );
+ }
+ if (delta == 0) {
+ return;
+ }
+
+ ((ResizableCacheEntry) entry).resizeReservation(newSize);
Review Comment:
[P2] Update held weak-byte accounting when shrinking
`addWeakReservationHold` records `currHoldBytes` using the weak entry's size
at hold acquisition. If that held weak entry later calls `adjustReservation`,
this code shrinks `currWeakSizeBytes` and the entry size but leaves
`currHoldBytes` at the old value; when the hold closes, `trackWeakRelease`
subtracts only the new smaller size, permanently inflating
`VirtualStorageLocationStats.getHoldBytes()`. This affects the new partial
metadata flow whenever a weak-reserved metadata entry shrinks while its
bootstrap/acquire hold is active. The shrink path needs to adjust hold bytes
for active holds, and the tests should cover held weak entries.
--
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]
