>From Ali Alsuliman <[email protected]>: Ali Alsuliman has uploaded this change for review. ( https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/21677?usp=email )
Change subject: [ASTERIXDB-3817][CLOUD] De-duplicate the local-only write path ...................................................................... [ASTERIXDB-3817][CLOUD] De-duplicate the local-only write path AbstractCloudIOManager carried three methods with the same body, localIoManager.syncWrite(...): localWriter(ByteBuffer) and both overloads of localWriteOnly. "Write the local copy and do not upload" now has one implementation per buffer shape and the localWriteOnly overrides are one-line delegations to it. The reason the duplication appeared is an asymmetry on ICloudIOManager: cloudWrite is declared for both a single ByteBuffer and a ByteBuffer[], but localWriter only for the single buffer. The gathering shape is the one the uncompressed buffer-cache path actually takes, since a page is written together with its header, so a caller needing "local only" for that shape had nothing to call. Declare localWriter(ByteBuffer[]) beside its cloudWrite counterpart and the seam is complete. IOManager.localWriteOnly stays, and its javadoc now says why rather than leaving it to be asked again. The behaviour is reachable through ICloudIOManager, but the dispatch is not: the caller is LocalOnlyWriteContext, which must live in hyracks-storage-common because hyracks-storage-am-vtree selects it, and hyracks-cloud depends on hyracks-storage-common rather than the reverse -- so unlike DefaultCloudOnlyWriteContext it cannot name ICloudIOManager. A virtual method on IOManager is therefore the only available seam. The exit is recorded too: if the vector index ever gains a cloud-aware layer that can inject a write context the way IColumnIndexDiskCacheManager does for column indexes, that method and both of its overrides can go. No behaviour change. The 31 LSM VTree tests cover the local path; the S3 and Azure cloud suites pass 4/4 each. LSMGCSTest fails in setup on a ConnectException against the mock GCS server it expects on 127.0.0.1, which is absent on this host and unrelated to this change. Ext-ref: MB-73194 Co-Authored-By: Claude Opus 5 <[email protected]> Change-Id: I723eaa5b947ab9be7630c046e775b2c0606ec14d --- M asterixdb/asterix-cloud/src/main/java/org/apache/asterix/cloud/AbstractCloudIOManager.java M hyracks-fullstack/hyracks/hyracks-cloud/src/main/java/org/apache/hyracks/cloud/io/ICloudIOManager.java M hyracks-fullstack/hyracks/hyracks-control/hyracks-control-nc/src/main/java/org/apache/hyracks/control/nc/io/IOManager.java M hyracks-fullstack/hyracks/hyracks-storage-common/src/main/java/org/apache/hyracks/storage/common/buffercache/context/write/LocalOnlyWriteContext.java 4 files changed, 63 insertions(+), 9 deletions(-) git pull ssh://asterix-gerrit.ics.uci.edu:29418/asterixdb refs/changes/77/21677/1 diff --git a/asterixdb/asterix-cloud/src/main/java/org/apache/asterix/cloud/AbstractCloudIOManager.java b/asterixdb/asterix-cloud/src/main/java/org/apache/asterix/cloud/AbstractCloudIOManager.java index 2e22005..6829e39 100644 --- a/asterixdb/asterix-cloud/src/main/java/org/apache/asterix/cloud/AbstractCloudIOManager.java +++ b/asterixdb/asterix-cloud/src/main/java/org/apache/asterix/cloud/AbstractCloudIOManager.java @@ -66,6 +66,7 @@ import org.apache.hyracks.cloud.io.stream.CloudInputStream; import org.apache.hyracks.cloud.util.CloudRetryableRequestUtil; import org.apache.hyracks.control.nc.io.IOManager; +import org.apache.hyracks.util.annotations.AiProvenance; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; @@ -248,16 +249,29 @@ return localIoManager.syncWrite(fHandle, offset, data); } + @AiProvenance(agent = AiProvenance.Agent.CLAUDE_OPUS_5, tool = AiProvenance.Tool.CLAUDE_CODE_UI) @Override - public final int localWriteOnly(IFileHandle fHandle, long offset, ByteBuffer data) throws HyracksDataException { - // Local copy only; the long-term (cloud) upload is intentionally deferred to a later pass. + public final long localWriter(IFileHandle fHandle, long offset, ByteBuffer[] data) throws HyracksDataException { + // Using syncWrite here to avoid closing the file channel when the thread is interrupted return localIoManager.syncWrite(fHandle, offset, data); } + /** + * Deliberately identical to {@link #localWriter(IFileHandle, long, ByteBuffer)}: writing the local copy + * and not uploading is one behaviour, and this is the {@code IOManager} override through which callers + * that cannot see {@link ICloudIOManager} reach it. See {@code IOManager#localWriteOnly} for why that + * indirection exists. + */ + @AiProvenance(agent = AiProvenance.Agent.CLAUDE_OPUS_5, tool = AiProvenance.Tool.CLAUDE_CODE_UI, contributionKind = AiProvenance.ContributionKind.REFACTORED, notes = "Delegate to localWriter") + @Override + public final int localWriteOnly(IFileHandle fHandle, long offset, ByteBuffer data) throws HyracksDataException { + return localWriter(fHandle, offset, data); + } + + @AiProvenance(agent = AiProvenance.Agent.CLAUDE_OPUS_5, tool = AiProvenance.Tool.CLAUDE_CODE_UI, contributionKind = AiProvenance.ContributionKind.REFACTORED, notes = "Delegate to localWriter") @Override public final long localWriteOnly(IFileHandle fHandle, long offset, ByteBuffer[] data) throws HyracksDataException { - // Local copy only; the long-term (cloud) upload is intentionally deferred to a later pass. - return localIoManager.syncWrite(fHandle, offset, data); + return localWriter(fHandle, offset, data); } @Override diff --git a/hyracks-fullstack/hyracks/hyracks-cloud/src/main/java/org/apache/hyracks/cloud/io/ICloudIOManager.java b/hyracks-fullstack/hyracks/hyracks-cloud/src/main/java/org/apache/hyracks/cloud/io/ICloudIOManager.java index c21589a..63cd287 100644 --- a/hyracks-fullstack/hyracks/hyracks-cloud/src/main/java/org/apache/hyracks/cloud/io/ICloudIOManager.java +++ b/hyracks-fullstack/hyracks/hyracks-cloud/src/main/java/org/apache/hyracks/cloud/io/ICloudIOManager.java @@ -29,6 +29,7 @@ import org.apache.hyracks.cloud.io.request.ICloudRequest; import org.apache.hyracks.cloud.io.stream.CloudInputStream; import org.apache.hyracks.cloud.util.CloudRetryableRequestUtil; +import org.apache.hyracks.util.annotations.AiProvenance; /** * Certain operations needed to be provided by {@link org.apache.hyracks.api.io.IIOManager} to support cloud @@ -76,6 +77,20 @@ int localWriter(IFileHandle fHandle, long offset, ByteBuffer data) throws HyracksDataException; /** + * Write to local drive only, gathering from several buffers — the shape the buffer cache uses when a + * page is written together with its header. Mirrors the two shapes of + * {@link #cloudWrite(IFileHandle, long, ByteBuffer[])} so that "local only" and "cloud only" are + * expressible for the same writes. + * + * @param fHandle file handle + * @param offset starting offset + * @param data to write + * @return number of written bytes + */ + @AiProvenance(agent = AiProvenance.Agent.CLAUDE_OPUS_5, tool = AiProvenance.Tool.CLAUDE_CODE_UI) + long localWriter(IFileHandle fHandle, long offset, ByteBuffer[] data) throws HyracksDataException; + + /** * Write to cloud only * * @param fHandle file handle diff --git a/hyracks-fullstack/hyracks/hyracks-control/hyracks-control-nc/src/main/java/org/apache/hyracks/control/nc/io/IOManager.java b/hyracks-fullstack/hyracks/hyracks-control/hyracks-control-nc/src/main/java/org/apache/hyracks/control/nc/io/IOManager.java index 0d23dde..52f0f98 100644 --- a/hyracks-fullstack/hyracks/hyracks-control/hyracks-control-nc/src/main/java/org/apache/hyracks/control/nc/io/IOManager.java +++ b/hyracks-fullstack/hyracks/hyracks-control/hyracks-control-nc/src/main/java/org/apache/hyracks/control/nc/io/IOManager.java @@ -265,16 +265,36 @@ } /** - * Writes to local storage only, deliberately skipping any long-term (cloud) upload. Goes through - * {@link #syncWrite} (the IoRequest path) rather than {@link #doSyncWrite} so an interrupt on the - * calling thread does not close the file channel via {@code ClosedByInterruptException}; cloud IO - * managers override it to write the local copy without uploading. Used by multi-pass writers (e.g. - * the VTree static-structure builder) that publish a page locally first and upload it later, once. + * Writes to local storage only, deliberately skipping any long-term (cloud) upload. Off-cloud that is + * an ordinary local write; the cloud IO managers override both overloads to write the local copy and + * not upload. Used by multi-pass writers (the VTree static-structure builder) that publish a page + * locally, mutate it, and upload it exactly once in a later pass — necessary because the cloud writer + * is append-only, so a page must not be uploaded before its final bytes are known. + * <p> + * Goes through {@link #syncWrite} (the IoRequest path) rather than {@link #doSyncWrite} so an + * interrupt on the calling thread does not close the file channel via + * {@code ClosedByInterruptException}. + * <p> + * <b>Why this lives here rather than only on the cloud manager.</b> The behaviour itself already + * exists as {@code ICloudIOManager#localWriter}, and the cloud overrides of this method are one-line + * delegations to it. What cannot be delegated is the <em>dispatch</em>: the caller is + * {@code LocalOnlyWriteContext}, which must live in {@code hyracks-storage-common} because + * {@code hyracks-storage-am-vtree} selects it, and {@code hyracks-cloud} depends on + * {@code hyracks-storage-common} rather than the reverse — so that context cannot name + * {@code ICloudIOManager} the way {@code DefaultCloudOnlyWriteContext} does. A virtual method here is + * therefore the seam. The alternative is the one the column indexes use: have a cloud-aware layer + * inject a cloud-specific write context ({@code CloudColumnWriteContext}); if the vector index ever + * grows such a layer, this method and its overrides can go. */ public int localWriteOnly(IFileHandle fHandle, long offset, ByteBuffer data) throws HyracksDataException { return syncWrite(fHandle, offset, data); } + /** + * Gathering-write counterpart of {@link #localWriteOnly(IFileHandle, long, ByteBuffer)}. This is the + * overload the uncompressed buffer-cache path actually takes, since a page is written together with + * its header. + */ public long localWriteOnly(IFileHandle fHandle, long offset, ByteBuffer[] data) throws HyracksDataException { return syncWrite(fHandle, offset, data); } diff --git a/hyracks-fullstack/hyracks/hyracks-storage-common/src/main/java/org/apache/hyracks/storage/common/buffercache/context/write/LocalOnlyWriteContext.java b/hyracks-fullstack/hyracks/hyracks-storage-common/src/main/java/org/apache/hyracks/storage/common/buffercache/context/write/LocalOnlyWriteContext.java index 5acf50a..c47dc77 100644 --- a/hyracks-fullstack/hyracks/hyracks-storage-common/src/main/java/org/apache/hyracks/storage/common/buffercache/context/write/LocalOnlyWriteContext.java +++ b/hyracks-fullstack/hyracks/hyracks-storage-common/src/main/java/org/apache/hyracks/storage/common/buffercache/context/write/LocalOnlyWriteContext.java @@ -32,6 +32,11 @@ * Intended for the first pass of a multi-pass writer that publishes pages locally, mutates them, and * uploads them exactly once in a later pass — required because the cloud writer is append-only, so a * page must not be uploaded before its final bytes are known. + * <p> + * Unlike {@code DefaultCloudOnlyWriteContext}, this one cannot cast the {@code IOManager} to + * {@code ICloudIOManager} and call {@code localWriter} directly: it has to be usable in both deployments, + * and this module cannot depend on {@code hyracks-cloud}, which depends on it. Hence the dispatch through + * a virtual {@code IOManager} method whose cloud overrides delegate to {@code localWriter}. */ public final class LocalOnlyWriteContext implements IBufferCacheWriteContext { public static final IBufferCacheWriteContext INSTANCE = new LocalOnlyWriteContext(); -- To view, visit https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/21677?usp=email To unsubscribe, or for help writing mail filters, visit https://asterix-gerrit.ics.uci.edu/settings?usp=email Gerrit-MessageType: newchange Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Change-Id: I723eaa5b947ab9be7630c046e775b2c0606ec14d Gerrit-Change-Number: 21677 Gerrit-PatchSet: 1 Gerrit-Owner: Ali Alsuliman <[email protected]>
