Skip to content
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions api/src/main/java/org/apache/iceberg/SnapshotUpdate.java
Original file line number Diff line number Diff line change
Expand Up @@ -60,4 +60,10 @@ public interface SnapshotUpdate<ThisT> extends PendingUpdate<Snapshot> {
* @return this for method chaining
*/
ThisT scanManifestsWith(ExecutorService executorService);

/**
Comment thread
namrathamyske marked this conversation as resolved.
* Perform operations on a particular branch
* @param branch which is name of SnapshotRef of type branch.
*/
ThisT toBranch(String branch);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs a default that throws UnsupportedOperationException.

}
40 changes: 36 additions & 4 deletions core/src/main/java/org/apache/iceberg/SnapshotProducer.java
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ public void accept(String file) {
private Consumer<String> deleteFunc = defaultDelete;

Comment thread
namrathamyske marked this conversation as resolved.
Comment thread
namrathamyske marked this conversation as resolved.
private ExecutorService workerPool = ThreadPools.getWorkerPool();
private String targetBranch = SnapshotRef.MAIN_BRANCH;

protected SnapshotProducer(TableOperations ops) {
this.ops = ops;
Expand Down Expand Up @@ -116,6 +117,38 @@ public ThisT scanManifestsWith(ExecutorService executorService) {
return self();
}

@Override
public ThisT toBranch(String branch) {
throw new UnsupportedOperationException("Performing operations on a branch is currently not supported");

@rdblue rdblue Jul 31, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: I think this is a good time to use this.getClass().getName() so that it's clear which operation doesn't support committing to a branch.

throw new UnsupportedOperationException(Sting.format(
    "Cannot commit to branch %s: %s does not support branch commits",
    branch,
    this.getClass().getName());

}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be in the interface, not here.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point, fixed


/***
* Will be used by snapshot producer operations to create a new ref if an invalid branch is passed
* @param branch ref name on which operation is to performed
*/
protected void createNewRef(String branch) {
SnapshotRef branchRef = SnapshotRef.branchBuilder(this.current().currentSnapshot().snapshotId()).build();
TableMetadata.Builder updatedBuilder = TableMetadata.buildFrom(this.current());
updatedBuilder.setRef(branch, branchRef);
ops.commit(ops.current(), updatedBuilder.build());

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Committing to a branch while creating it should not commit twice. Instead, the commit should add a snapshot and then create a branch pointing to that snapshot.

@amogh-jahagirdar amogh-jahagirdar Aug 1, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah if we're creating a new branch, the commit for the snapshot producer needs to encapsulate the producing of the snapshot and the branch creation

}

/***
* A setter for the target branch on which snapshot producer operation should be performed
* @param branch to set as target branch
*/
protected void setTargetBranch(String branch) {
this.targetBranch = branch;
}

/***
* A getter for the target branch on which snapshot producer operation should be performed
* @return target branch
*/
protected String getTargetBranch() {

@rdblue rdblue Jul 31, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Iceberg typically does not use get in method names because it is useless. Either it should be replaced by a more specific verb or it should be omitted.

Also, what is the purpose of this method? None of the other configurations are exposed back to the caller. Do we need this at all?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see from #5010 that this is used in each child implementation to get the parent snapshot.

I think it would be better for maintainability and future implementations if the apply method were passed the parent snapshot.

return targetBranch;
}

protected ExecutorService workerPool() {
return this.workerPool;
}
Expand Down Expand Up @@ -167,8 +200,7 @@ protected void validate(TableMetadata currentMetadata) {
@Override
public Snapshot apply() {
refresh();
Long parentSnapshotId = base.currentSnapshot() != null ?
base.currentSnapshot().snapshotId() : null;
Long parentSnapshotId = base.ref(targetBranch) != null ? base.ref(targetBranch).snapshotId() : null;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be the existing branch state, the current snapshot (if creating the branch), or null.

long sequenceNumber = base.nextSequenceNumber();

// run validations from the child operation

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to pass the parent snapshot ID into validate along with table metadata (base) so that validations can check changes between the starting snapshot ID and the parent snapshot, rather than between the starting snapshot ID and the current snapshot.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same thing with apply.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rdblue My thought process is to check the whether starting snapshot ID is an ancestor of parent snapshot. If not then throw invalid starting snapshot ID exception. Else we should go ahead with checking between starting snapshot ID & parent ID.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/namrathamyske/iceberg/pull/1/files should take care of what @rdblue is talking about here which if I'm not mistaken is more about a cleaner abstraction for applying a change to a specific snapshot, and running a validation to a specific snapshot which will be needed when we implement branch support for the other producers.

Expand Down Expand Up @@ -298,11 +330,11 @@ public void commit() {
TableMetadata.Builder update = TableMetadata.buildFrom(base);
if (base.snapshot(newSnapshot.snapshotId()) != null) {
// this is a rollback operation
update.setBranchSnapshot(newSnapshot.snapshotId(), SnapshotRef.MAIN_BRANCH);
Comment thread
namrathamyske marked this conversation as resolved.
Outdated
update.setBranchSnapshot(newSnapshot.snapshotId(), targetBranch);
} else if (stageOnly) {
update.addSnapshot(newSnapshot);
} else {
update.setBranchSnapshot(newSnapshot, SnapshotRef.MAIN_BRANCH);
update.setBranchSnapshot(newSnapshot, targetBranch);
}

TableMetadata updated = update.build();
Expand Down
9 changes: 9 additions & 0 deletions core/src/test/java/org/apache/iceberg/TestFastAppend.java
Original file line number Diff line number Diff line change
Expand Up @@ -478,4 +478,13 @@ public void testIncludedPartitionSummaryLimit() {
String changedPartitions = table.currentSnapshot().summary().get(SnapshotSummary.CHANGED_PARTITION_COUNT_PROP);
Assert.assertEquals("Should set changed partition count", "2", changedPartitions);
}

@Test(expected = UnsupportedOperationException.class)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To test exceptions, we use either TestHelpers.assertThrows or Assertions. That's important so that you can add assertions about the error message, and so that you can run assertions to validate that state did not change. In this case, the the branch name should not exist and the snapshot list should be the same size.

public void testAppendToBranch() throws UnsupportedOperationException {
table.newFastAppend()
.appendFile(FILE_A)
Comment thread
namrathamyske marked this conversation as resolved.
Outdated
.commit();
table.manageSnapshots().createBranch("ref", table.currentSnapshot().snapshotId()).commit();
table.newFastAppend().appendFile(FILE_B).toBranch("ref").commit();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this implement branch commits for FastAppend? That way we can see one operation that works from end-to-end when reviewing the changes to SnapshotUpdate.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rdblue Have written the complete test-case in #5010.

}
}