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 2021/05/03 19:34:51 UTC
[GitHub] [iceberg] flyrain opened a new pull request #2552: Core: Fix the NPE while updating event in the context of eventual consistency.
flyrain opened a new pull request #2552:
URL: https://github.com/apache/iceberg/pull/2552
We need the same thing as [here](https://github.com/apache/iceberg/blob/83886c87f516afed7e625bed7f5749838217138c/core/src/main/java/org/apache/iceberg/SnapshotProducer.java#L315) in the context of eventual consistency. Otherwise, we hits a NPE like this. We need a fix even though it is a WARN.
```
timestamp="2021-04-23T22:35:23,938+0000",level="WARN",threadName="pool-1-thread-1",appName="spark-driver",logger="org.apache.iceberg.SnapshotProducer",message="Failed to notify listeners",exception="java.lang.NullPointerException
at org.apache.iceberg.MergingSnapshotProducer.updateEvent(MergingSnapshotProducer.java:377)
at org.apache.iceberg.SnapshotProducer.notifyListeners(SnapshotProducer.java:329)
at org.apache.iceberg.SnapshotProducer.commit(SnapshotProducer.java:324)
at org.apache.iceberg.spark.source.SparkWrite.commitOperation(SparkWrite.java:236)
```
--
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.
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 change in pull request #2552: Core: Fix the NPE while updating event in the context of eventual consistency.
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #2552:
URL: https://github.com/apache/iceberg/pull/2552#discussion_r635554518
##########
File path: core/src/main/java/org/apache/iceberg/MergingSnapshotProducer.java
##########
@@ -390,7 +394,16 @@ protected void validateDataFilesExist(TableMetadata base, Long startingSnapshotI
@Override
public Object updateEvent() {
long snapshotId = snapshotId();
- long sequenceNumber = ops.refresh().snapshot(snapshotId).sequenceNumber();
+ Snapshot justSaved = ops.refresh().snapshot(snapshotId);
+ long sequenceNumber = TableMetadata.INVALID_SEQUENCE_NUMBER;
+ if (justSaved == null) {
+ // The snapshot just saved may not be present if the latest metadata couldn't be loaded due to eventual
+ // consistency problems in refresh.
+ LOG.warn("Failed to load committed snapshot, leave its sequence number to an invalid number(-1)");
Review comment:
No need for back-off. That is handled by the table operations when it tries to load the new metadata. All this needs to do is handle the case where the table metadata can't be loaded.
--
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.
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] flyrain commented on a change in pull request #2552: Core: Fix the NPE while updating event in the context of eventual consistency.
Posted by GitBox <gi...@apache.org>.
flyrain commented on a change in pull request #2552:
URL: https://github.com/apache/iceberg/pull/2552#discussion_r628646974
##########
File path: core/src/main/java/org/apache/iceberg/MergingSnapshotProducer.java
##########
@@ -390,7 +394,16 @@ protected void validateDataFilesExist(TableMetadata base, Long startingSnapshotI
@Override
public Object updateEvent() {
long snapshotId = snapshotId();
- long sequenceNumber = ops.refresh().snapshot(snapshotId).sequenceNumber();
+ Snapshot justSaved = ops.refresh().snapshot(snapshotId);
+ long sequenceNumber = TableMetadata.INVALID_SEQUENCE_NUMBER;
+ if (justSaved == null) {
+ // The snapshot just saved may not be present if the latest metadata couldn't be loaded due to eventual
+ // consistency problems in refresh.
+ LOG.warn("Failed to load committed snapshot, leave its sequence number to an invalid number(-1)");
Review comment:
There is a retry in the main part of method `commit()`, not for clean-up and `notifyListeners`. I'm not sure if it's a good idea to introduce retry to them. I didn't see much downside, at most delay the commit, but feel like it is a bit over engineering. It is going to be the another PR anyway.
--
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.
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 change in pull request #2552: Core: Fix the NPE while updating event in the context of eventual consistency.
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #2552:
URL: https://github.com/apache/iceberg/pull/2552#discussion_r635555862
##########
File path: core/src/main/java/org/apache/iceberg/MergingSnapshotProducer.java
##########
@@ -390,7 +394,16 @@ protected void validateDataFilesExist(TableMetadata base, Long startingSnapshotI
@Override
public Object updateEvent() {
long snapshotId = snapshotId();
- long sequenceNumber = ops.refresh().snapshot(snapshotId).sequenceNumber();
+ Snapshot justSaved = ops.refresh().snapshot(snapshotId);
+ long sequenceNumber = TableMetadata.INVALID_SEQUENCE_NUMBER;
+ if (justSaved == null) {
+ // The snapshot just saved may not be present if the latest metadata couldn't be loaded due to eventual
+ // consistency problems in refresh.
+ LOG.warn("Failed to load committed snapshot, leave its sequence number to an invalid number(-1)");
Review comment:
I think we can improve the message; "leave its sequence number to an invalid number(-1)" isn't very clear. How about "Failed to load committed snapshot: omitting sequence number from notifications (not known)"
--
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.
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 #2552: Core: Fix the NPE while updating event in the context of eventual consistency.
Posted by GitBox <gi...@apache.org>.
rdblue merged pull request #2552:
URL: https://github.com/apache/iceberg/pull/2552
--
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.
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] flyrain commented on a change in pull request #2552: Core: Fix the NPE while updating event in the context of eventual consistency.
Posted by GitBox <gi...@apache.org>.
flyrain commented on a change in pull request #2552:
URL: https://github.com/apache/iceberg/pull/2552#discussion_r635612266
##########
File path: core/src/main/java/org/apache/iceberg/MergingSnapshotProducer.java
##########
@@ -390,7 +394,16 @@ protected void validateDataFilesExist(TableMetadata base, Long startingSnapshotI
@Override
public Object updateEvent() {
long snapshotId = snapshotId();
- long sequenceNumber = ops.refresh().snapshot(snapshotId).sequenceNumber();
+ Snapshot justSaved = ops.refresh().snapshot(snapshotId);
+ long sequenceNumber = TableMetadata.INVALID_SEQUENCE_NUMBER;
+ if (justSaved == null) {
+ // The snapshot just saved may not be present if the latest metadata couldn't be loaded due to eventual
+ // consistency problems in refresh.
+ LOG.warn("Failed to load committed snapshot, leave its sequence number to an invalid number(-1)");
Review comment:
Fixed it in a new commit.
##########
File path: core/src/main/java/org/apache/iceberg/BaseSnapshot.java
##########
@@ -87,7 +85,7 @@
String operation,
Map<String, String> summary,
List<ManifestFile> dataManifests) {
- this(io, INITIAL_SEQUENCE_NUMBER, snapshotId, parentId, timestampMillis, operation, summary, null);
+ this(io, TableMetadata.INITIAL_SEQUENCE_NUMBER, snapshotId, parentId, timestampMillis, operation, summary, null);
Review comment:
Removed it in a new commit.
--
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.
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] kbendick commented on a change in pull request #2552: Core: Fix the NPE while updating event in the context of eventual consistency.
Posted by GitBox <gi...@apache.org>.
kbendick commented on a change in pull request #2552:
URL: https://github.com/apache/iceberg/pull/2552#discussion_r628622660
##########
File path: core/src/main/java/org/apache/iceberg/MergingSnapshotProducer.java
##########
@@ -390,7 +394,16 @@ protected void validateDataFilesExist(TableMetadata base, Long startingSnapshotI
@Override
public Object updateEvent() {
long snapshotId = snapshotId();
- long sequenceNumber = ops.refresh().snapshot(snapshotId).sequenceNumber();
+ Snapshot justSaved = ops.refresh().snapshot(snapshotId);
+ long sequenceNumber = TableMetadata.INVALID_SEQUENCE_NUMBER;
+ if (justSaved == null) {
+ // The snapshot just saved may not be present if the latest metadata couldn't be loaded due to eventual
+ // consistency problems in refresh.
+ LOG.warn("Failed to load committed snapshot, leave its sequence number to an invalid number(-1)");
Review comment:
As a possible follow up, should we consider retrying this with a backoff (or does this already do that)? Or is that something we'd rather avoid getting into?
--
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.
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 #2552: Core: Fix the NPE while updating event in the context of eventual consistency.
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on pull request #2552:
URL: https://github.com/apache/iceberg/pull/2552#issuecomment-832279226
I think this is a good fix to the NPE. But I do want to make sure we warn folks that an eventually consistent Catalog can break the history of an Iceberg table. This would only be acceptable for a catalog which guaranteed consistent reads during the lock phase.
My thinking is basically
```
Consistent - Read Your Writes Catalog = Best
Eventually Consistent Reads - Consistent when locking/committing = Adequate but not great
Eventually consistent during lock/commit = Will cause lost writes, non-linearizable histories and weirdness.
```
--
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.
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 change in pull request #2552: Core: Fix the NPE while updating event in the context of eventual consistency.
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #2552:
URL: https://github.com/apache/iceberg/pull/2552#discussion_r635550852
##########
File path: core/src/main/java/org/apache/iceberg/BaseSnapshot.java
##########
@@ -87,7 +85,7 @@
String operation,
Map<String, String> summary,
List<ManifestFile> dataManifests) {
- this(io, INITIAL_SEQUENCE_NUMBER, snapshotId, parentId, timestampMillis, operation, summary, null);
+ this(io, TableMetadata.INITIAL_SEQUENCE_NUMBER, snapshotId, parentId, timestampMillis, operation, summary, null);
Review comment:
This change looks unrelated to me. Can you remove it?
--
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.
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] flyrain commented on a change in pull request #2552: Core: Fix the NPE while updating event in the context of eventual consistency.
Posted by GitBox <gi...@apache.org>.
flyrain commented on a change in pull request #2552:
URL: https://github.com/apache/iceberg/pull/2552#discussion_r625440611
##########
File path: core/src/main/java/org/apache/iceberg/MergingSnapshotProducer.java
##########
@@ -390,7 +394,16 @@ protected void validateDataFilesExist(TableMetadata base, Long startingSnapshotI
@Override
public Object updateEvent() {
long snapshotId = snapshotId();
- long sequenceNumber = ops.refresh().snapshot(snapshotId).sequenceNumber();
+ Snapshot justSaved = ops.refresh().snapshot(snapshotId);
+ long sequenceNumber = 0;
+ if( justSaved == null) {
+ // The snapshot just saved may not be present if the latest metadata couldn't be loaded due to eventual
+ // consistency problems in refresh.
+ LOG.warn("Failed to load committed snapshot, leave sequence number to 0");
+ } else {
+ sequenceNumber = justSaved.sequenceNumber();
Review comment:
Good catch. Make sense to set it to -1 since its initial value is 0 and increase monotonically as this line shows https://github.com/apache/iceberg/blob/9edb1fabc0a4ad4c3f10625c4a15918bf5790f8b/core/src/main/java/org/apache/iceberg/TableMetadata.java#L356. Will post a new commit.
--
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.
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] flyrain commented on a change in pull request #2552: Core: Fix the NPE while updating event in the context of eventual consistency.
Posted by GitBox <gi...@apache.org>.
flyrain commented on a change in pull request #2552:
URL: https://github.com/apache/iceberg/pull/2552#discussion_r625440611
##########
File path: core/src/main/java/org/apache/iceberg/MergingSnapshotProducer.java
##########
@@ -390,7 +394,16 @@ protected void validateDataFilesExist(TableMetadata base, Long startingSnapshotI
@Override
public Object updateEvent() {
long snapshotId = snapshotId();
- long sequenceNumber = ops.refresh().snapshot(snapshotId).sequenceNumber();
+ Snapshot justSaved = ops.refresh().snapshot(snapshotId);
+ long sequenceNumber = 0;
+ if( justSaved == null) {
+ // The snapshot just saved may not be present if the latest metadata couldn't be loaded due to eventual
+ // consistency problems in refresh.
+ LOG.warn("Failed to load committed snapshot, leave sequence number to 0");
+ } else {
+ sequenceNumber = justSaved.sequenceNumber();
Review comment:
Good catch. Make sense to set it to -1 since its initial value is 0 and increase monotonically as this line shows https://github.com/apache/iceberg/blob/9edb1fabc0a4ad4c3f10625c4a15918bf5790f8b/core/src/main/java/org/apache/iceberg/TableMetadata.java#L356
Posted a new commit to fix it.
--
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.
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] flyrain commented on a change in pull request #2552: Core: Fix the NPE while updating event in the context of eventual consistency.
Posted by GitBox <gi...@apache.org>.
flyrain commented on a change in pull request #2552:
URL: https://github.com/apache/iceberg/pull/2552#discussion_r628646974
##########
File path: core/src/main/java/org/apache/iceberg/MergingSnapshotProducer.java
##########
@@ -390,7 +394,16 @@ protected void validateDataFilesExist(TableMetadata base, Long startingSnapshotI
@Override
public Object updateEvent() {
long snapshotId = snapshotId();
- long sequenceNumber = ops.refresh().snapshot(snapshotId).sequenceNumber();
+ Snapshot justSaved = ops.refresh().snapshot(snapshotId);
+ long sequenceNumber = TableMetadata.INVALID_SEQUENCE_NUMBER;
+ if (justSaved == null) {
+ // The snapshot just saved may not be present if the latest metadata couldn't be loaded due to eventual
+ // consistency problems in refresh.
+ LOG.warn("Failed to load committed snapshot, leave its sequence number to an invalid number(-1)");
Review comment:
There is a retry in the main part of method `commit()`, not for clean-up and `notifyListeners`. I'm not sure if it's a good idea to introduce retry to them. I didn't see much downside, at most delay the commit, but feel like it is a bit over engineering. It is going to be the next PR anyway.
--
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.
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 #2552: Core: Fix the NPE while updating event in the context of eventual consistency.
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #2552:
URL: https://github.com/apache/iceberg/pull/2552#issuecomment-844599303
Thanks, @flyrain!
--
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.
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 change in pull request #2552: Core: Fix the NPE while updating event in the context of eventual consistency.
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #2552:
URL: https://github.com/apache/iceberg/pull/2552#discussion_r635553771
##########
File path: core/src/main/java/org/apache/iceberg/MergingSnapshotProducer.java
##########
@@ -390,7 +394,16 @@ protected void validateDataFilesExist(TableMetadata base, Long startingSnapshotI
@Override
public Object updateEvent() {
long snapshotId = snapshotId();
- long sequenceNumber = ops.refresh().snapshot(snapshotId).sequenceNumber();
+ Snapshot justSaved = ops.refresh().snapshot(snapshotId);
+ long sequenceNumber = TableMetadata.INVALID_SEQUENCE_NUMBER;
Review comment:
Nevermind, I see the comment about not being able to load the metadata.
I'd probably prefer null, but -1 is fine so that we don't change a public API.
--
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.
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] flyrain commented on a change in pull request #2552: Core: Fix the NPE while updating event in the context of eventual consistency.
Posted by GitBox <gi...@apache.org>.
flyrain commented on a change in pull request #2552:
URL: https://github.com/apache/iceberg/pull/2552#discussion_r635561926
##########
File path: core/src/main/java/org/apache/iceberg/BaseSnapshot.java
##########
@@ -87,7 +85,7 @@
String operation,
Map<String, String> summary,
List<ManifestFile> dataManifests) {
- this(io, INITIAL_SEQUENCE_NUMBER, snapshotId, parentId, timestampMillis, operation, summary, null);
+ this(io, TableMetadata.INITIAL_SEQUENCE_NUMBER, snapshotId, parentId, timestampMillis, operation, summary, null);
Review comment:
Trying to piggy-back a change to consolidate `INITIAL_SEQUENCE_NUMBER` from multiple places. I can put it in another PR if it is preferred.
--
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.
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] yyanyy commented on a change in pull request #2552: Core: Fix the NPE while updating event in the context of eventual consistency.
Posted by GitBox <gi...@apache.org>.
yyanyy commented on a change in pull request #2552:
URL: https://github.com/apache/iceberg/pull/2552#discussion_r625375478
##########
File path: core/src/main/java/org/apache/iceberg/MergingSnapshotProducer.java
##########
@@ -390,7 +394,16 @@ protected void validateDataFilesExist(TableMetadata base, Long startingSnapshotI
@Override
public Object updateEvent() {
long snapshotId = snapshotId();
- long sequenceNumber = ops.refresh().snapshot(snapshotId).sequenceNumber();
+ Snapshot justSaved = ops.refresh().snapshot(snapshotId);
+ long sequenceNumber = 0;
+ if( justSaved == null) {
+ // The snapshot just saved may not be present if the latest metadata couldn't be loaded due to eventual
+ // consistency problems in refresh.
+ LOG.warn("Failed to load committed snapshot, leave sequence number to 0");
+ } else {
+ sequenceNumber = justSaved.sequenceNumber();
Review comment:
I think the fix looks good but I'm not sure if we should default `sequenceNumber` to 0, as this may result in incorrect data for users that rely on this sequence number in the event. Since without this change we probably will not send out the event at all, if we don't want to wait until the data become eventual I wonder if returning -1 would be better since 0 may refer to an actual sequence number of the table (although in v1 all sequence number will be 0 so return 0 for v1 is better, but I'm not sure how easy it is to check table version here)
--
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.
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] flyrain commented on pull request #2552: Core: Fix the NPE while updating event in the context of eventual consistency.
Posted by GitBox <gi...@apache.org>.
flyrain commented on pull request #2552:
URL: https://github.com/apache/iceberg/pull/2552#issuecomment-833707758
User should avoid eventually consistent Catalog if possible. If Hive catalog is used, we normally expect HMS is strong consistency. It is NOT the case some times, for example, HMS could be an eventual consistency system if it uses Cassandra as the backend DB. But I like the call-out from @RussellSpitzer and @kbendick, avoid it as much as possible.
However, we cannot remove eventual consistency related code if Iceberg still support Hadoop catalog, which based on an eventual consistency system, HDFS.
--
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.
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 change in pull request #2552: Core: Fix the NPE while updating event in the context of eventual consistency.
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #2552:
URL: https://github.com/apache/iceberg/pull/2552#discussion_r635606717
##########
File path: core/src/main/java/org/apache/iceberg/BaseSnapshot.java
##########
@@ -87,7 +85,7 @@
String operation,
Map<String, String> summary,
List<ManifestFile> dataManifests) {
- this(io, INITIAL_SEQUENCE_NUMBER, snapshotId, parentId, timestampMillis, operation, summary, null);
+ this(io, TableMetadata.INITIAL_SEQUENCE_NUMBER, snapshotId, parentId, timestampMillis, operation, summary, null);
Review comment:
Yes, please do. We try to avoid unnecessary changes so that we don't cause unnecessary commit conflicts for people maintaining branches.
--
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.
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] aokolnychyi commented on a change in pull request #2552: Core: Fix the NPE while updating event in the context of eventual consistency.
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on a change in pull request #2552:
URL: https://github.com/apache/iceberg/pull/2552#discussion_r634706478
##########
File path: core/src/main/java/org/apache/iceberg/MergingSnapshotProducer.java
##########
@@ -390,7 +394,16 @@ protected void validateDataFilesExist(TableMetadata base, Long startingSnapshotI
@Override
public Object updateEvent() {
long snapshotId = snapshotId();
- long sequenceNumber = ops.refresh().snapshot(snapshotId).sequenceNumber();
+ Snapshot justSaved = ops.refresh().snapshot(snapshotId);
+ long sequenceNumber = TableMetadata.INVALID_SEQUENCE_NUMBER;
Review comment:
Well, I am not sure what should be the best way to react here. We do expect the catalog to be consistent so one option is to just log an error and not throw an event or throw some special even type.
cc @rdsr who implemented this part and @rdblue who uses the event-based system. What do you expect in this case?
--
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.
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 change in pull request #2552: Core: Fix the NPE while updating event in the context of eventual consistency.
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #2552:
URL: https://github.com/apache/iceberg/pull/2552#discussion_r635551857
##########
File path: core/src/main/java/org/apache/iceberg/MergingSnapshotProducer.java
##########
@@ -390,7 +394,16 @@ protected void validateDataFilesExist(TableMetadata base, Long startingSnapshotI
@Override
public Object updateEvent() {
long snapshotId = snapshotId();
- long sequenceNumber = ops.refresh().snapshot(snapshotId).sequenceNumber();
+ Snapshot justSaved = ops.refresh().snapshot(snapshotId);
+ long sequenceNumber = TableMetadata.INVALID_SEQUENCE_NUMBER;
Review comment:
What is the situation where the snapshot would not be in metadata? If the snapshot wasn't committed, then I would expect to not send an event.
--
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.
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] kbendick commented on pull request #2552: Core: Fix the NPE while updating event in the context of eventual consistency.
Posted by GitBox <gi...@apache.org>.
kbendick commented on pull request #2552:
URL: https://github.com/apache/iceberg/pull/2552#issuecomment-833238321
> I think this is a good fix to the NPE. But I do want to make sure we warn folks that an eventually consistent Catalog can break the history of an Iceberg table. This would only be acceptable for a catalog which guaranteed consistent reads during the lock phase.
>
> My thinking is basically
>
> ```
> Consistent - Read Your Writes Catalog = Best
> Eventually Consistent Reads - Consistent when locking/committing = Adequate but not great
> Eventually consistent during lock/commit = Will cause lost writes, non-linearizable histories and weirdness.
> ```
Is this something that could come up with the DynamoDB based lock? I agree this is a good fix to the NPE, but I have personally had weird edge cases with other software that uses a DynamoDB table as a lock (such as terraform). Ideally testing handles this and the DynamoDB / Glue catalog doesn't suffer from these issues. Possibly I need to review the DynamoDB table lock again, but felt it might be worth noting as I've had weird issues with terraform table locks and consistency issues.
--
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.
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] flyrain commented on pull request #2552: Core: Fix the NPE while updating event in the context of eventual consistency.
Posted by GitBox <gi...@apache.org>.
flyrain commented on pull request #2552:
URL: https://github.com/apache/iceberg/pull/2552#issuecomment-844539911
The test failures are unrelated.
--
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.
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] flyrain commented on pull request #2552: Core: Fix the NPE while updating event in the context of eventual consistency.
Posted by GitBox <gi...@apache.org>.
flyrain commented on pull request #2552:
URL: https://github.com/apache/iceberg/pull/2552#issuecomment-831611181
Can you take a look? @rdblue @aokolnychyi @RussellSpitzer @karuppayya @kbendick
--
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.
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