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 2020/06/11 19:12:43 UTC
[GitHub] [iceberg] rdsr opened a new issue #1109: TableMetadata#lastUpdatedMillis can end up with lower timestamp value compared to the existing previous metadata-log entries
rdsr opened a new issue #1109:
URL: https://github.com/apache/iceberg/issues/1109
In our environment, expiry of snapshots and table updates are happening from multiple machines. Recently we saw the following exception
```
java.lang.IllegalArgumentException: [BUG] Expected sorted previous metadata log entries.
10-06-2020 15:04:31 PDT streaming-test-data-cleaner INFO - at org.apache.iceberg.shaded.com.google.common.base.Preconditions.checkArgument(Preconditions.java:142)
10-06-2020 15:04:31 PDT streaming-test-data-cleaner INFO - at org.apache.iceberg.TableMetadata.<init>(TableMetadata.java:231)
10-06-2020 15:04:31 PDT streaming-test-data-cleaner INFO - at org.apache.iceberg.TableMetadata.removeSnapshotsIf(TableMetadata.java:436)
```
On checking the metadata.json file, we see that `TableMetadata#lastUpdatedMillis` has `last-updated-ms: 1591804947492`, whereas the latest entry in the `metadata-log` has `timestamp-ms : 1591804951286` . It seems to me that the when calling `TableMetadata#removeSnapshotsIf()`, the `System.currentTimestampMillis` computed is system dependent and can have a lower timestamp compared to existing metadata-log entries. This can cause further updates to metadata.json to fail because of the sorted check in the constructor of TableMetadata
----------------------------------------------------------------
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] rdsr commented on issue #1109: TableMetadata#lastUpdatedMillis can end up with lower timestamp value compared to the existing previous metadata-log entries
Posted by GitBox <gi...@apache.org>.
rdsr commented on issue #1109:
URL: https://github.com/apache/iceberg/issues/1109#issuecomment-642955020
> We'll also have to remove the ordering done when we read from metadata files if we allow skew, to avoid reordering valid entries.
I see that you pointed to `SnapshotLogEntry` . Do you want to address SnapshotLog also as part of this?
----------------------------------------------------------------
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 issue #1109: TableMetadata#lastUpdatedMillis can end up with lower timestamp value compared to the existing previous metadata-log entries
Posted by GitBox <gi...@apache.org>.
rdblue commented on issue #1109:
URL: https://github.com/apache/iceberg/issues/1109#issuecomment-642924875
I'm fine making it configurable, but I prefer avoiding configuration if it isn't needed. We could do something as big as 1 minute, I just want to catch cases where clocks are significantly off.
We'll also have to remove the [ordering done when we read from metadata files](https://github.com/apache/iceberg/blob/master/core/src/main/java/org/apache/iceberg/TableMetadataParser.java#L296) if we allow skew, to avoid reordering valid entries.
I think we do want to check the log entries every time we create a TableMetadata instance, so that we detect cases of large clock skew as soon as possible.
----------------------------------------------------------------
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] rdsr commented on issue #1109: TableMetadata#lastUpdatedMillis can end up with lower timestamp value compared to the existing previous metadata-log entries
Posted by GitBox <gi...@apache.org>.
rdsr commented on issue #1109:
URL: https://github.com/apache/iceberg/issues/1109#issuecomment-642917053
Also, the latest lastUpdatedMillis can be lower from the metadata log entry timestamps. That should also be checked.
In our case the clock skew was around 3 secs
Side question, if we always check that the lastUpdatedMillis field is >= the latest previous log entry with a tolerance, We don't need to check all the log entries every time we create a TableMetadata instance
----------------------------------------------------------------
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] rdsr commented on issue #1109: TableMetadata#lastUpdatedMillis can end up with lower timestamp value compared to the existing previous metadata-log entries
Posted by GitBox <gi...@apache.org>.
rdsr commented on issue #1109:
URL: https://github.com/apache/iceberg/issues/1109#issuecomment-642878970
----------------------------------------------------------------
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 issue #1109: TableMetadata#lastUpdatedMillis can end up with lower timestamp value compared to the existing previous metadata-log entries
Posted by GitBox <gi...@apache.org>.
rdblue commented on issue #1109:
URL: https://github.com/apache/iceberg/issues/1109#issuecomment-643433927
I think we need to update both the snapshot and metadata logs.
I think the metadata log could still have out of order entries if other writers are incorrect compared to the local system time. And there could also be writers that are not the Java Iceberg implementation.
----------------------------------------------------------------
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] rdsr edited a comment on issue #1109: TableMetadata#lastUpdatedMillis can end up with lower timestamp value compared to the existing previous metadata-log entries
Posted by GitBox <gi...@apache.org>.
rdsr edited a comment on issue #1109:
URL: https://github.com/apache/iceberg/issues/1109#issuecomment-642955020
> We'll also have to remove the ordering done when we read from metadata files if we allow skew, to avoid reordering valid entries.
I see that you pointed to `SnapshotLogEntry` . Do you want to address SnapshotLog also as part of this?
> I think we do want to check the log entries every time we create a TableMetadata instance, so that we detect cases of large clock skew as soon as possible.
My point was that since we will always check TableMetadat#lastUpdatedMillis >= latest(metadata-log entry ts) the log will never have out of order entries persisted in the metadata.json to being with
----------------------------------------------------------------
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 issue #1109: TableMetadata#lastUpdatedMillis can end up with lower timestamp value compared to the existing previous metadata-log entries
Posted by GitBox <gi...@apache.org>.
rdblue commented on issue #1109:
URL: https://github.com/apache/iceberg/issues/1109#issuecomment-642912064
Yeah, unfortunately this can happen for a number of reasons.
I considered the option of modifying `lastUpdatedMillis` so that it is always strictly increasing, but that could lead to some really bad behaviors. For example, if a node is misconfigured and its timestamp is a year in the future, we don't want to either make up a timestamp like `max(lastUpdated+1, now)` because that perpetuates the problem. We also can't just wait until `now > lastUpdated` because that might be a long time and would DOS the table. What I concluded was that we might have timestamps that are off and that has to be okay.
Given that, I think that the current check for `lastUpdatedMillis` in `TableMetadata` should be considered the bug. Since we know this can happen, we should allow for some reasonable clock skew in that check.
How about changing the check to allow some out-of-order log entries, but nothing more than 1s?
```java
HistoryEntry last = null;
for (HistoryEntry logEntry : snapshotLog) {
if (last != null) {
Preconditions.checkArgument(
(logEntry.timestampMillis() - last.timestampMillis()) >= -1000,
"[BUG] Expected sorted snapshot log entries.");
}
last = logEntry;
}
```
----------------------------------------------------------------
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] rdsr edited a comment on issue #1109: TableMetadata#lastUpdatedMillis can end up with lower timestamp value compared to the existing previous metadata-log entries
Posted by GitBox <gi...@apache.org>.
rdsr edited a comment on issue #1109:
URL: https://github.com/apache/iceberg/issues/1109#issuecomment-642917053
Also, the latest lastUpdatedMillis can be lower from the metadata log entry timestamps. That should also be checked.
In our case the clock skew was around 3 secs, so 1 sec may be too small a value. Would you consider making this configurable?
Side question, if we always check that the lastUpdatedMillis field is >= the latest previous log entry with a tolerance, We don't need to check all the log entries every time we create a TableMetadata instance
----------------------------------------------------------------
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] rdsr edited a comment on issue #1109: TableMetadata#lastUpdatedMillis can end up with lower timestamp value compared to the existing previous metadata-log entries
Posted by GitBox <gi...@apache.org>.
rdsr edited a comment on issue #1109:
URL: https://github.com/apache/iceberg/issues/1109#issuecomment-642955020
> We'll also have to remove the ordering done when we read from metadata files if we allow skew, to avoid reordering valid entries.
I see that you pointed to `SnapshotLogEntry` . Do you want to address SnapshotLog also as part of this?
> I think we do want to check the log entries every time we create a TableMetadata instance, so that we detect cases of large clock skew as soon as possible.
My point was that since we will always check TableMetadat#lastUpdatedMillis >= latest(metadata-log entry ts) the log will never have out of order entries to being with
----------------------------------------------------------------
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] rdsr edited a comment on issue #1109: TableMetadata#lastUpdatedMillis can end up with lower timestamp value compared to the existing previous metadata-log entries
Posted by GitBox <gi...@apache.org>.
rdsr edited a comment on issue #1109:
URL: https://github.com/apache/iceberg/issues/1109#issuecomment-642955020
> We'll also have to remove the ordering done when we read from metadata files if we allow skew, to avoid reordering valid entries.
I see that you pointed to `SnapshotLogEntry` . Do you want to address SnapshotLog also as part of this?
----------------------------------------------------------------
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 closed issue #1109: TableMetadata#lastUpdatedMillis can end up with lower timestamp value compared to the existing previous metadata-log entries
Posted by GitBox <gi...@apache.org>.
rdblue closed issue #1109:
URL: https://github.com/apache/iceberg/issues/1109
----------------------------------------------------------------
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