You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2022/05/11 12:39:17 UTC
[GitHub] [iceberg] ajantha-bhat opened a new pull request, #4747: Core: Fix data loss when duplicate snapshot-id is allocated
ajantha-bhat opened a new pull request, #4747:
URL: https://github.com/apache/iceberg/pull/4747
There is astronomically low probability of allocating same snapshot id.
But as we are converting 128 bit UUID to 64 bit uuid, snapshot id is not as unique as UUID.
If the duplicate snapshot ids are allocated, Iceberg doesn't handle it and it leads to the data loss.
TODO: This PR only reproduces issue. Yet to fix.
I am not sure whether to throw exception during commit or do metadata retry with new snapshot id.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on pull request #4747: Core: Fix data loss when duplicate snapshot-id is allocated
Posted by GitBox <gi...@apache.org>.
rdblue commented on PR #4747:
URL: https://github.com/apache/iceberg/pull/4747#issuecomment-1124244241
Looks like what's happening is that the snapshot is never added. It hits this check: https://github.com/apache/iceberg/blob/master/core/src/main/java/org/apache/iceberg/TableMetadata.java#L984-L985
I think we can fix this by changing `SnapshotProducer`:
```diff
+ while (snapshotId == null || ops.current().snapshot(snapshotId) != null) {
- if (snapshotId != null) {
this.snapshotId = ops.newSnapshotId();
}
```
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] RussellSpitzer commented on pull request #4747: Core: Fix data loss when duplicate snapshot-id is allocated
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on PR #4747:
URL: https://github.com/apache/iceberg/pull/4747#issuecomment-1126806592
I still think the probability of collision here is probably worse than serial lottery wins statistically, but this should be a relatively cheap check so It's probably fine to do
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #4747: Core: Fix data loss when duplicate snapshot-id is allocated
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #4747:
URL: https://github.com/apache/iceberg/pull/4747#discussion_r873064062
##########
core/src/main/java/org/apache/iceberg/TableMetadata.java:
##########
@@ -980,11 +980,15 @@ public Builder addSortOrder(SortOrder order) {
}
public Builder addSnapshot(Snapshot snapshot) {
- if (snapshot == null || snapshotsById.containsKey(snapshot.snapshotId())) {
+ if (snapshot == null) {
// change is a noop
return this;
}
+ ValidationException.check(!snapshotsById.containsKey(snapshot.snapshotId()),
+ "Snapshot already exist for the id: %s",
Review Comment:
`exist` -> `exists`
##########
core/src/main/java/org/apache/iceberg/TableMetadata.java:
##########
@@ -980,11 +980,15 @@ public Builder addSortOrder(SortOrder order) {
}
public Builder addSnapshot(Snapshot snapshot) {
- if (snapshot == null || snapshotsById.containsKey(snapshot.snapshotId())) {
+ if (snapshot == null) {
// change is a noop
return this;
}
+ ValidationException.check(!snapshotsById.containsKey(snapshot.snapshotId()),
+ "Snapshot already exist for the id: %s",
Review Comment:
`exist` -> `exists`. Also, remove "the"
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] ajantha-bhat commented on pull request #4747: Core: Fix data loss when duplicate snapshot-id is allocated
Posted by GitBox <gi...@apache.org>.
ajantha-bhat commented on PR #4747:
URL: https://github.com/apache/iceberg/pull/4747#issuecomment-1124769091
> I think we can fix this by changing SnapshotProducer:
@rdblue: Fixed it as per your suggestion. I too expected this kind of check initially.
Should we do this check at the commit time and throw exception? Now, concurrent table operation getting (both are uncommitted) getting the same snapshot id is not handled I guess.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue merged pull request #4747: Core: Fix data loss when duplicate snapshot-id is allocated
Posted by GitBox <gi...@apache.org>.
rdblue merged PR #4747:
URL: https://github.com/apache/iceberg/pull/4747
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] ajantha-bhat commented on pull request #4747: Core: Fix data loss when duplicate snapshot-id is allocated
Posted by GitBox <gi...@apache.org>.
ajantha-bhat commented on PR #4747:
URL: https://github.com/apache/iceberg/pull/4747#issuecomment-1124891437
> Now, concurrent table operation getting (both are uncommitted) getting the same snapshot id is not handled I guess.
Handled concurrent scenario by throwing validation exception in TableMetadata preparation.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] ajantha-bhat commented on a diff in pull request #4747: Core: Fix data loss when duplicate snapshot-id is allocated
Posted by GitBox <gi...@apache.org>.
ajantha-bhat commented on code in PR #4747:
URL: https://github.com/apache/iceberg/pull/4747#discussion_r870253324
##########
core/src/main/java/org/apache/iceberg/SnapshotIdGeneratorUtil.java:
##########
@@ -0,0 +1,40 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you 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 org.apache.iceberg;
+
+import java.util.UUID;
+
+public final class SnapshotIdGeneratorUtil {
Review Comment:
I just moved snapshot id generation logic to static method, so that I can use MockStatic.
Normal object level mock or spy is impossible to use for an end to end SQL testcase as `Table Operations` objects keeps on changing. Hence, static using static mock.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #4747: Core: Fix data loss when duplicate snapshot-id is allocated
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #4747:
URL: https://github.com/apache/iceberg/pull/4747#discussion_r873064050
##########
core/src/main/java/org/apache/iceberg/TableMetadata.java:
##########
@@ -980,11 +980,15 @@ public Builder addSortOrder(SortOrder order) {
}
public Builder addSnapshot(Snapshot snapshot) {
- if (snapshot == null || snapshotsById.containsKey(snapshot.snapshotId())) {
+ if (snapshot == null) {
// change is a noop
return this;
}
+ ValidationException.check(!snapshotsById.containsKey(snapshot.snapshotId()),
+ "Snapshot already exist for the id: %s",
+ snapshot.snapshotId());
Review Comment:
@RussellSpitzer, what do you think about this? Any situations where you think this would cause an issue because we add the same snapshot and rely on the operation being idempotent?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] ajantha-bhat commented on pull request #4747: Core: Fix data loss when duplicate snapshot-id is allocated
Posted by GitBox <gi...@apache.org>.
ajantha-bhat commented on PR #4747:
URL: https://github.com/apache/iceberg/pull/4747#issuecomment-1134792155
@rdblue, @RussellSpitzer: Can we merge this PR?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] ajantha-bhat commented on pull request #4747: Core: Fix data loss when duplicate snapshot-id is allocated
Posted by GitBox <gi...@apache.org>.
ajantha-bhat commented on PR #4747:
URL: https://github.com/apache/iceberg/pull/4747#issuecomment-1147070474
@rdblue , @RussellSpitzer : Ping...
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org