-
Notifications
You must be signed in to change notification settings - Fork 1.9k
[Kernel] [CatalogManaged] New Transaction and Committer etc. APIs #4814
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
[Kernel] [CatalogManaged] New Transaction and Committer etc. APIs #4814
Conversation
Committer
, CommitPayload
, CommitResponse
, CommitFailedException
APIs
kernel/kernel-api/src/main/java/io/delta/kernel/commit/CommitFailedException.java
Outdated
Show resolved
Hide resolved
kernel/kernel-api/src/main/java/io/delta/kernel/commit/CommitPayload.java
Outdated
Show resolved
Hide resolved
* The {@link Protocol} that was read at the beginning of the commit. Empty if a new table is | ||
* being created. | ||
*/ | ||
public final Optional<Protocol> readProtocolOpt; |
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.
Can you remind me why we need the old/new p and ms?
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.
Good question! For example, to detect that the catalogManaged
table feature has been enabled on the table.
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.
And, certainly, you want the new Protocol and Metadata so you can send that to the catalog, and then it always knows what's the latets P & M, and it can give that back on the next read request to efficiently construct a ResolvedTable without having to go to the file system to resolve the latest P & M
b603c47
to
072ede5
Compare
kernel/kernel-api/src/main/java/io/delta/kernel/commit/CommitFailedException.java
Outdated
Show resolved
Hide resolved
kernel/kernel-api/src/main/java/io/delta/kernel/commit/CommitPayload.java
Outdated
Show resolved
Hide resolved
kernel/kernel-api/src/main/java/io/delta/kernel/commit/CommitPayload.java
Outdated
Show resolved
Hide resolved
76c689f
to
75bae78
Compare
Committer
, CommitPayload
, CommitResponse
, CommitFailedException
APIs
kernel/kernel-api/src/main/java/io/delta/kernel/commit/CommitFailedException.java
Show resolved
Hide resolved
kernel/kernel-api/src/main/java/io/delta/kernel/commit/CommitMetadata.java
Show resolved
Hide resolved
kernel/kernel-api/src/main/java/io/delta/kernel/commit/CommitMetadata.java
Show resolved
Hide resolved
kernel/kernel-api/src/main/java/io/delta/kernel/commit/CommitMetadata.java
Outdated
Show resolved
Hide resolved
kernel/kernel-api/src/main/java/io/delta/kernel/transaction/TransactionV2.java
Outdated
Show resolved
Hide resolved
kernel/kernel-api/src/main/java/io/delta/kernel/commit/CommitMetadata.java
Show resolved
Hide resolved
04565d9
to
230779b
Compare
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.
looks good! Just a nit.
kernel/kernel-api/src/main/java/io/delta/kernel/transaction/TransactionV2.java
Outdated
Show resolved
Hide resolved
…DefaultCommitter skeleton (#4936) ## 🥞 Stacked PR Use this [link](https://github.com/delta-io/delta/pull/4936/files) to review incremental changes. - [**stack/kernel_catalog_managed_writes_get_committer**](#4936) [[Files changed](https://github.com/delta-io/delta/pull/4936/files)] --------- <!-- Thanks for sending a pull request! Here are some tips for you: 1. If this is your first time, please read our contributor guidelines: https://github.com/delta-io/delta/blob/master/CONTRIBUTING.md 2. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP] Your PR title ...'. 3. Be sure to keep the PR description updated to reflect all changes. 4. Please write your PR title to summarize what this PR proposes. 5. If possible, provide a concise example to reproduce the issue for a faster review. 6. If applicable, include the corresponding issue number in the PR title and link it in the body. --> #### Which Delta project/connector is this regarding? - [ ] Spark - [ ] Standalone - [ ] Flink - [X] Kernel - [ ] Other (fill in here) ## Description #4814 introduced new TransactionV2, Committer, CommitContext, etc APIs. This PR now adds the abilities for Engines to inject into Kernel the actual Committer. This PR also includes a no-op Default committer implementation that validates that it is not interacting with catalog-managed tables. We do this because: ResolvedTable does not return an Optional committer, but rather just a committer. So we need to give it _something_ in the case that no committer was provided to the table builder. ## How was this patch tested? New UTs. ## Does this PR introduce _any_ user-facing changes? New .withCommitter API
🥞 Stacked PR
Use this link to review incremental changes.
Which Delta project/connector is this regarding?
Description
This PR adds new Transaction and Committer etc. APIs.
How was this patch tested?
Just new APIs.
Does this PR introduce any user-facing changes?
Yes.