[Kernel] Update the Kernel read with coordinated commit support #3381
Conversation
This file contains 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
EstherBear
force-pushed
the
cc-read
branch
2 times, most recently
from
272f6e3
to
181a900
Compare
8 tasks
EstherBear
force-pushed
the
cc-read
branch
6 times, most recently
from
4549bf5
to
03ade37
Compare
kernel/kernel-api/src/main/java/io/delta/kernel/internal/snapshot/SnapshotManager.java
Outdated
Show resolved
Hide resolved
kernel/kernel-api/src/main/java/io/delta/kernel/internal/util/FileNames.java
Outdated
Show resolved
Hide resolved
kernel/kernel-api/src/main/java/io/delta/kernel/internal/snapshot/SnapshotManager.java
Outdated
Show resolved
Hide resolved
kernel/kernel-api/src/main/java/io/delta/kernel/internal/SnapshotImpl.java
Outdated
Show resolved
Hide resolved
kernel/kernel-api/src/main/java/io/delta/kernel/internal/snapshot/SnapshotManager.java
Outdated
Show resolved
Hide resolved
kernel/kernel-api/src/main/java/io/delta/kernel/internal/snapshot/SnapshotManager.java
Outdated
Show resolved
Hide resolved
kernel/kernel-api/src/main/java/io/delta/kernel/internal/snapshot/SnapshotManager.java
Outdated
Show resolved
Hide resolved
kernel/kernel-api/src/main/java/io/delta/kernel/internal/snapshot/SnapshotManager.java
Outdated
Show resolved
Hide resolved
kernel/kernel-api/src/main/java/io/delta/kernel/internal/snapshot/SnapshotManager.java
Outdated
Show resolved
Hide resolved
kernel/kernel-api/src/main/java/io/delta/kernel/internal/snapshot/SnapshotManager.java
Outdated
Show resolved
Hide resolved
...lts/src/main/java/io/delta/kernel/defaults/engine/DefaultCommitCoordinatorClientHandler.java
Outdated
Show resolved
Hide resolved
...est/scala/io/delta/kernel/defaults/internal/coordinatedcommits/CoordinatedCommitsSuite.scala
Outdated
Show resolved
Hide resolved
kernel/kernel-api/src/main/java/io/delta/kernel/internal/snapshot/SnapshotManager.java
Outdated
Show resolved
Hide resolved
...est/scala/io/delta/kernel/defaults/internal/coordinatedcommits/CoordinatedCommitsSuite.scala
Outdated
Show resolved
Hide resolved
...est/scala/io/delta/kernel/defaults/internal/coordinatedcommits/CoordinatedCommitsSuite.scala
Show resolved
Hide resolved
...est/scala/io/delta/kernel/defaults/internal/coordinatedcommits/CoordinatedCommitsSuite.scala
Show resolved
Hide resolved
...est/scala/io/delta/kernel/defaults/internal/coordinatedcommits/CoordinatedCommitsSuite.scala
Outdated
Show resolved
Hide resolved
...est/scala/io/delta/kernel/defaults/internal/coordinatedcommits/CoordinatedCommitsSuite.scala
Outdated
Show resolved
Hide resolved
kernel/kernel-api/src/main/java/io/delta/kernel/internal/snapshot/SnapshotManager.java
Outdated
Show resolved
Hide resolved
kernel/kernel-api/src/main/java/io/delta/kernel/internal/snapshot/SnapshotManager.java
Outdated
Show resolved
Hide resolved
kernel/kernel-api/src/main/java/io/delta/kernel/internal/snapshot/SnapshotManager.java
Outdated
Show resolved
Hide resolved
...est/scala/io/delta/kernel/defaults/internal/coordinatedcommits/CoordinatedCommitsSuite.scala
Show resolved
Hide resolved
vkorukanti
requested changes
Jul 26, 2024
kernel/kernel-api/src/main/java/io/delta/kernel/internal/SnapshotImpl.java
Outdated
Show resolved
Hide resolved
kernel/kernel-api/src/main/java/io/delta/kernel/internal/SnapshotImpl.java
Outdated
Show resolved
Hide resolved
Comment on lines
321
to
329
| List<Commit> unbackfilledCommits = commitCoordinatorClientHandlerOpt | ||
| .map(commitCoordinatorClientHandler -> commitCoordinatorClientHandler | ||
| .getCommits( | ||
| logPath.toString(), | ||
| coordinatedCommitsTableConf, | ||
| startVersion, | ||
| versionToLoad.orElse(null)) | ||
| .getCommits()) | ||
| .orElse(Collections.emptyList()); |
There was a problem hiding this comment.
How do you handle errors here?
For example the commit coordinator fails due to an intermittent issue or misconfiguration, do you fail the query?
There was a problem hiding this comment.
Also can you move this a separate method add long some log statements before making a request to commit coordinator. You can actually follow the log statements added in Delta-Spark.
There was a problem hiding this comment.
I don't think we have checked exception for the getCommits API. But I've added some log statements for it.
kernel/kernel-api/src/main/java/io/delta/kernel/internal/snapshot/SnapshotManager.java
Outdated
Show resolved
Hide resolved
kernel/kernel-api/src/main/java/io/delta/kernel/internal/snapshot/SnapshotManager.java
Outdated
Show resolved
Hide resolved
kernel/kernel-api/src/main/java/io/delta/kernel/internal/snapshot/SnapshotManager.java
Outdated
Show resolved
Hide resolved
kernel/kernel-api/src/main/java/io/delta/kernel/internal/util/FileNames.java
Outdated
Show resolved
Hide resolved
kernel/kernel-api/src/test/scala/io/delta/kernel/internal/SnapshotManagerSuite.scala
Outdated
Show resolved
Hide resolved
vkorukanti
requested changes
Jul 29, 2024
kernel/kernel-api/src/main/java/io/delta/kernel/internal/snapshot/SnapshotManager.java
Outdated
Show resolved
Hide resolved
kernel/kernel-api/src/main/java/io/delta/kernel/internal/snapshot/SnapshotManager.java
Outdated
Show resolved
Hide resolved
kernel/kernel-api/src/main/java/io/delta/kernel/internal/snapshot/SnapshotManager.java
Outdated
Show resolved
Hide resolved
kernel/kernel-api/src/main/java/io/delta/kernel/internal/snapshot/SnapshotManager.java
Outdated
Show resolved
Hide resolved
kernel/kernel-api/src/main/java/io/delta/kernel/internal/snapshot/SnapshotManager.java
Outdated
Show resolved
Hide resolved
kernel/kernel-api/src/main/java/io/delta/kernel/internal/snapshot/SnapshotManager.java
Outdated
Show resolved
Hide resolved
kernel/kernel-api/src/main/java/io/delta/kernel/internal/snapshot/SnapshotManager.java
Outdated
Show resolved
Hide resolved
kernel/kernel-api/src/main/java/io/delta/kernel/internal/snapshot/SnapshotManager.java
Outdated
Show resolved
Hide resolved
...api/src/main/java/io/delta/kernel/internal/snapshot/TableCommitCoordinatorClientHandler.java
Show resolved
Hide resolved
| public TableCommitCoordinatorClientHandler( | ||
| CommitCoordinatorClientHandler commitCoordinatorClientHandler, | ||
| String logPath, | ||
| Map<String, String> tableConf) { |
There was a problem hiding this comment.
@dhruvarya-db @prakharjain09 Why do we need to pass the tableConf to each API on the commit coordinator? Isn't it used when creating the commit coordinator?
There was a problem hiding this comment.
@vkorukanti Commit Coordinator is supposed to be reuseable across multiple tables (like JSONHandler). tableConf can be thought of as a tablePath/tableId which is used to uniquely identify the table.
Open
8 tasks
dhruvarya-db
approved these changes
Jul 29, 2024
vkorukanti
requested changes
Jul 29, 2024
kernel/kernel-api/src/main/java/io/delta/kernel/internal/SnapshotImpl.java
Outdated
Show resolved
Hide resolved
kernel/kernel-api/src/main/java/io/delta/kernel/internal/snapshot/SnapshotManager.java
Outdated
Show resolved
Hide resolved
kernel/kernel-api/src/main/java/io/delta/kernel/internal/snapshot/SnapshotManager.java
Outdated
Show resolved
Hide resolved
kernel/kernel-api/src/main/java/io/delta/kernel/internal/snapshot/SnapshotManager.java
Outdated
Show resolved
Hide resolved
kernel/kernel-api/src/main/java/io/delta/kernel/internal/snapshot/SnapshotManager.java
Outdated
Show resolved
Hide resolved
kernel/kernel-api/src/main/java/io/delta/kernel/internal/snapshot/SnapshotManager.java
Outdated
Show resolved
Hide resolved
...est/scala/io/delta/kernel/defaults/internal/coordinatedcommits/CoordinatedCommitsSuite.scala
Outdated
Show resolved
Hide resolved
vkorukanti
reviewed
Jul 30, 2024
kernel/kernel-api/src/main/java/io/delta/kernel/internal/snapshot/SnapshotManager.java
Outdated
Show resolved
Hide resolved
kernel/kernel-api/src/main/java/io/delta/kernel/internal/snapshot/SnapshotManager.java
Outdated
Show resolved
Hide resolved
kernel/kernel-api/src/main/java/io/delta/kernel/internal/snapshot/SnapshotManager.java
Outdated
Show resolved
Hide resolved
dhruvarya-db
approved these changes
Aug 2, 2024
kernel/kernel-api/src/main/java/io/delta/kernel/internal/snapshot/SnapshotManager.java
Outdated
Show resolved
Hide resolved
kernel/kernel-api/src/main/java/io/delta/kernel/internal/snapshot/SnapshotManager.java
Outdated
Show resolved
Hide resolved
kernel/kernel-api/src/main/java/io/delta/kernel/internal/snapshot/SnapshotManager.java
Outdated
Show resolved
Hide resolved
EstherBear
force-pushed
the
cc-read
branch
5 times, most recently
from
80e6690
to
533e438
Compare
vkorukanti
requested changes
Aug 5, 2024
kernel/kernel-api/src/main/java/io/delta/kernel/internal/snapshot/SnapshotManager.java
Outdated
Show resolved
Hide resolved
kernel/kernel-api/src/main/java/io/delta/kernel/internal/SnapshotImpl.java
Outdated
Show resolved
Hide resolved
kernel/kernel-api/src/main/java/io/delta/kernel/internal/snapshot/SnapshotManager.java
Outdated
Show resolved
Hide resolved
kernel/kernel-api/src/main/java/io/delta/kernel/internal/snapshot/SnapshotManager.java
Outdated
Show resolved
Hide resolved
kernel/kernel-api/src/main/java/io/delta/kernel/internal/snapshot/SnapshotManager.java
Outdated
Show resolved
Hide resolved
kernel/kernel-api/src/test/scala/io/delta/kernel/internal/SnapshotManagerSuite.scala
Show resolved
Hide resolved
kernel/kernel-api/src/test/scala/io/delta/kernel/internal/SnapshotManagerSuite.scala
Outdated
Show resolved
Hide resolved
kernel/kernel-api/src/test/scala/io/delta/kernel/internal/SnapshotManagerSuite.scala
Show resolved
Hide resolved
...est/scala/io/delta/kernel/defaults/internal/coordinatedcommits/CoordinatedCommitsSuite.scala
Outdated
Show resolved
Hide resolved
...est/scala/io/delta/kernel/defaults/internal/coordinatedcommits/CoordinatedCommitsSuite.scala
Outdated
Show resolved
Hide resolved
| * This can be optimized by making snapshot hint optimization to work with coordinated commits. | ||
| * @see <a href="https://github.com/delta-io/delta/issues/3437">issue #3437</a>. | ||
| */ | ||
| private SnapshotImpl getCoordinatedCommitsAwareSnapshot( |
There was a problem hiding this comment.
For coordinated commit table then do we list the log dir twice? Can we not reuse the log segment we've already built? If this is because there may have been changes I'm wondering if it's possible (maybe to much additional complexity) to list from the last file we've seen and reuse existing segment.
There was a problem hiding this comment.
We must list the log dir twice otherwise there might be gap because of this issue.
There was a problem hiding this comment.
So the logic should be unbackfilled commit list -> deltalog list -> filter out duplicated unbackfilled commits using the results from deltalog list otherwise there might be gap or duplicated commits.
kernel/kernel-api/src/main/java/io/delta/kernel/internal/snapshot/SnapshotManager.java
Outdated
Show resolved
Hide resolved
vkorukanti
approved these changes
Aug 9, 2024
Comment on lines
+34
to
+36
| public static <T> T last(List<T> list) { | ||
| return list.get(list.size() - 1); | ||
| } |
There was a problem hiding this comment.
is it actually used? If not please remove it with a followup PR. We could remove in this PR itself, but will take another 4hr+ for the test to finish.
longvu-db
pushed a commit
to longvu-db/delta
that referenced
this pull request
Aug 13, 2024
…a-io#3381) ## Description Update the Kernel read with coordinated commit support and prepare for Kernel coordinated commit write support. ## How was this patch tested? Unit tests
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Which Delta project/connector is this regarding?
Description
Update the Kernel read with coordinated commit support and prepare for kernel coordinated commit write support.
How was this patch tested?
Unit tests
Does this PR introduce any user-facing changes?
Yes. Users can enable coordinated commit read support by configuring its properties.