gaborkaszab commented on code in PR #18073:
URL: https://github.com/apache/iceberg/pull/18073#discussion_r4442445933
##########
core/src/main/java/org/apache/iceberg/SnapshotProducer.java:
##########
@@ -685,7 +691,10 @@ protected ManifestReader<DeleteFile>
newDeleteManifestReader(ManifestFile manife
protected long snapshotId() {
if (snapshotId == null) {
synchronized (this) {
- while (snapshotId == null || ops.current().snapshot(snapshotId) !=
null) {
+ // only check ids that are already loaded: with lazily loaded
snapshots, a collision with
Review Comment:
I believe this code is needed because a `snapshot(id)` call would load the
missing snapshots. The contract for `SnapshotProducer.snapshotId()` function is
to get a snapshot Id that doesn't collide with any of the current snapshots.
The current change breaks this and puts responsibility to other parts of the
code.
Maybe even in REFS mode keep a set of taken snapshot IDs in `TableMetadata`?
We'd still need another API through `TableMetadata` to query if the ID is
taken, instead of calling `snapshot(id)`, but we'd keep the contract of this
function intact.
##########
core/src/main/java/org/apache/iceberg/rest/RESTTableOperations.java:
##########
@@ -286,17 +312,48 @@ private static Long
expectedSnapshotIdIfSnapshotAddOnly(List<MetadataUpdate> upd
}
private TableMetadata updateCurrentMetadata(LoadTableResponse response) {
+ return updateCurrentMetadata(response, false);
+ }
+
+ private TableMetadata updateCurrentMetadata(
+ LoadTableResponse response, boolean reinstallSnapshotsSupplier) {
// LoadTableResponse is used to deserialize the response, but config is
not allowed by the REST
// spec so it can be
// safely ignored. there is no requirement to update config on refresh or
commit.
if (current == null
|| !Objects.equals(current.metadataFileLocation(),
response.metadataLocation())) {
- this.current = checkUUID(current, response.tableMetadata());
+ TableMetadata refreshed = checkUUID(current, response.tableMetadata());
+ if (reinstallSnapshotsSupplier) {
Review Comment:
This seems redundant code with the same in `RESTSessionCatalog`. Change one
has the risk of forgetting to change the other. Would be nice to have a unified
way to add snapshot supplier to the table metadata. Maybe using a utility class
that can do that for us?
##########
core/src/main/java/org/apache/iceberg/TableMetadata.java:
##########
@@ -1000,18 +1034,27 @@ private Builder(TableMetadata base) {
this.sortOrders = Lists.newArrayList(base.sortOrders);
this.properties = Maps.newHashMap(base.properties);
this.currentSnapshotId = base.currentSnapshotId;
- this.snapshots = Lists.newArrayList(base.snapshots());
+
+ // copy the snapshot state as-is: when base loads snapshots lazily,
carry the supplier over
Review Comment:
If I don't get this wrong, the motivation of this part of the change is that
we want to avoid doing a full snapshot load when creating a new `TableMetadata`
from a base. I think that the PR is solving multiple problems, one is this and
the other is the REST ops related part. You might want to consider splitting
the PR by that boundary.
##########
core/src/main/java/org/apache/iceberg/TableMetadata.java:
##########
@@ -548,6 +559,19 @@ private synchronized void ensureSnapshotsLoaded() {
List<Snapshot> loadedSnapshots =
Lists.newArrayList(snapshotsSupplier.get());
loadedSnapshots.removeIf(s -> s.sequenceNumber() > lastSequenceNumber);
+ // retain snapshots that were added to this metadata but are unknown to
the supplier, such
Review Comment:
Not sure I follow why the below change is needed. Is this because previously
when we added a new snapshot we also did a full snapshot load and we put the
new snapshot on top? Do I get it right that previously when we had a
`MetadataUpdate.AddSnapshot` in `changes` we didn't reach this code because
`snapshotsLoaded` was true, and this changes now?
I feel that this part of the PR is not really intuitive and prone to
confusion.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
