-
Notifications
You must be signed in to change notification settings - Fork 156
Integration robustness contracts #710
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
jshook
wants to merge
10
commits into
main
Choose a base branch
from
integration-robustness
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
7c9375d
util/work: cooperative work-limiting and progress interface
jshook 15579ba
disk: pluggable output sink for compaction writes
jshook 914f83e
graph: ParallelExecutor abstraction for host-provided execution
jshook 6b528cb
util: RuntimeMode gate for opt-in diagnostic work
jshook d55d0d5
disk: memory-safety guards for host-managed reads
jshook d423dbf
Apply suggestion from @ashkrisk
jshook 0905091
util/work: move progress reporting onto PhaseScope
jshook 995bf22
disk: fix applied review suggestion
jshook d9ec26a
disk: rename Target to OutputReservation
jshook e4d9067
graph: ExecutorService adapter for ParallelExecutor
jshook File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
70 changes: 70 additions & 0 deletions
70
jvector-base/src/main/java/io/github/jbellis/jvector/disk/FileChannelSeekableSink.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,70 @@ | ||
| /* | ||
| * Copyright DataStax, Inc. | ||
| * | ||
| * Licensed 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 io.github.jbellis.jvector.disk; | ||
|
|
||
| import java.io.IOException; | ||
| import java.nio.ByteBuffer; | ||
| import java.nio.channels.FileChannel; | ||
|
|
||
| /** | ||
| * {@link SeekableSink} over a {@link FileChannel}, translating region-relative positions by a fixed | ||
| * base offset. The channel is owned by the caller; {@link #close()} does not close it. | ||
| */ | ||
| final class FileChannelSeekableSink implements SeekableSink { | ||
| private final FileChannel channel; | ||
| private final long baseOffset; | ||
|
|
||
| FileChannelSeekableSink(FileChannel channel, long baseOffset) { | ||
| if (channel == null) { | ||
| throw new NullPointerException("channel"); | ||
| } | ||
| if (baseOffset < 0) { | ||
| throw new IllegalArgumentException("baseOffset must be >= 0, got " + baseOffset); | ||
| } | ||
| this.channel = channel; | ||
| this.baseOffset = baseOffset; | ||
| } | ||
|
|
||
| @Override | ||
| public void writeAt(long position, ByteBuffer src) throws IOException { | ||
| if (position < 0) { | ||
| throw new IllegalArgumentException("position must be >= 0, got " + position); | ||
| } | ||
| long abs = baseOffset + position; | ||
| while (src.hasRemaining()) { | ||
| abs += channel.write(src, abs); | ||
| } | ||
| } | ||
|
|
||
| @Override | ||
| public int readAt(long position, ByteBuffer dst) throws IOException { | ||
| if (position < 0) { | ||
| throw new IllegalArgumentException("position must be >= 0, got " + position); | ||
| } | ||
| return channel.read(dst, baseOffset + position); | ||
| } | ||
|
|
||
| @Override | ||
| public void force() throws IOException { | ||
| channel.force(false); | ||
| } | ||
|
|
||
| @Override | ||
| public void close() { | ||
| // The channel is owned by the caller, per SeekableSink.over(...). | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
65 changes: 65 additions & 0 deletions
65
jvector-base/src/main/java/io/github/jbellis/jvector/disk/SeekableSink.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,65 @@ | ||
| /* | ||
| * Copyright DataStax, Inc. | ||
| * | ||
| * Licensed 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 io.github.jbellis.jvector.disk; | ||
|
|
||
| import io.github.jbellis.jvector.annotations.Experimental; | ||
|
|
||
| import java.io.IOException; | ||
| import java.nio.ByteBuffer; | ||
| import java.nio.channels.FileChannel; | ||
|
|
||
| /** | ||
| * A seekable region that supports positional reads and writes, addressed in coordinates relative | ||
| * to the region's start (0-based). An embedder uses it to hand a compactor (or other writer) a | ||
| * bounded window inside a larger container file: positions are region-relative and the | ||
| * implementation adds the container's base offset, so the writer never needs to know the absolute | ||
| * offset. | ||
| * | ||
| * <p>Implementations must support concurrent positional writes and reads to disjoint ranges (a | ||
| * {@link FileChannel} does). This is a generic IO primitive; the compaction extension point that | ||
| * hands one out is {@code io.github.jbellis.jvector.graph.disk.CompactionDestination}. | ||
| */ | ||
| @Experimental | ||
| public interface SeekableSink extends AutoCloseable { | ||
|
|
||
| /** Write {@code src} fully at region-relative {@code position} (must be {@code >= 0}). */ | ||
| void writeAt(long position, ByteBuffer src) throws IOException; | ||
|
|
||
| /** | ||
| * Read up to {@code dst.remaining()} bytes at region-relative {@code position} (must be | ||
| * {@code >= 0}); returns the number of bytes read, or {@code -1} at end of region. | ||
| */ | ||
| int readAt(long position, ByteBuffer dst) throws IOException; | ||
|
|
||
| /** Force written bytes to durable storage. */ | ||
| void force() throws IOException; | ||
|
|
||
| @Override | ||
| void close() throws IOException; | ||
|
|
||
| /** | ||
| * Reference implementation over a {@link FileChannel} region. Every region-relative position is | ||
| * translated by {@code baseOffset}. The channel's lifecycle is owned by the caller — this | ||
| * {@link #close()} does <b>not</b> close the channel. | ||
| * | ||
| * @param channel the backing channel, opened for read and write | ||
| * @param baseOffset the absolute offset of the region's start within {@code channel} ({@code >= 0}) | ||
| */ | ||
| static SeekableSink over(FileChannel channel, long baseOffset) { | ||
| return new FileChannelSeekableSink(channel, baseOffset); | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This interface appears to serve the same purpose as
RandomAccessReader/ReaderSupplier+RandomAccessWriter/IndexWriter. Is this intention to completely replace those existing interfaces with this one? If so, it would be helpful to jot down the advantages of this approach over what we already have.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is mostly captured in the javadoc above, but the main benefit here is to virtualize the output stream as logically owned by the jvector writer and physically owned by the embedding system. Specifically, this allows jvector to persist its indexes "care of" the owning system, which allows the writes to only happen once.
The alternative is that jvector owns the raw file management, and presumes everything to be jvector-only, full file ownership, 0-indexed. This is not true in practice, and by forcing it, we have caused other systems to have to copy and recopy data in order to properly virtualize it into the owning system's data formats and filesystem conventions.
The operative benefit is described in the javadoc above as:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is a more mechanical explanation of some of the decision points, with help from analysis:
RandomAccessReader/Writer are stateful cursor APIs (seek, then read/write sequentially), documented as not threadsafe, which is why ReaderSupplier exists to create one per thread. IndexWriter is a typed DataOutput serialization surface on top of that.
SeekableSink is a stateless positional-I/O primitive: writeAt/readAt with no cursor, where a single instance must support concurrent reads and writes to disjoint ranges. It also addresses a region — positions are relative to a base offset inside a caller-owned channel, and close() doesn't close the channel. That combination is what the CompactionDestination extension point needs: an embedder hands the compactor a bounded window inside its own container file (write the body, read it back for checksumming, on one handle) without exposing absolute offsets or giving up channel lifecycle.
None of that fits the existing interfaces without changing their documented threading/addressing contracts for every current implementation, so this is additive rather than a migration. If anything, the relationship is layered: a format-aware writer like IndexWriter could be implemented over a SeekableSink region.