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/04/22 09:06:52 UTC
[GitHub] [iceberg] kyle-cx opened a new pull request, #4612: Core:Optimize Error msg in validateDeletedManifests (fix #3466)
kyle-cx opened a new pull request, #4612:
URL: https://github.com/apache/iceberg/pull/4612
Error msg "Manifest is missing" in BaseRewriteManifests.validateDeletedManifests() is correct but a bit confusing, I optimize it to make user easier found out what happened. and now it looks like:
```
"Cannot apply RewriteManifests result, " +
"since some manifests being deleted have already been removed in current snapshot or not exist" +
" at all. current-snapshot-id:%d, missing-manifest-path:%s"
```
fix https://github.com/apache/iceberg/issues/3466
follow the suggestion from https://github.com/apache/iceberg/issues/3466#issuecomment-1104404998
cc: @RussellSpitzer
--
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] kyle-cx commented on a diff in pull request #4612: Core:Optimize Error msg in validateDeletedManifests (fix #3466)
Posted by GitBox <gi...@apache.org>.
kyle-cx commented on code in PR #4612:
URL: https://github.com/apache/iceberg/pull/4612#discussion_r856836859
##########
core/src/main/java/org/apache/iceberg/BaseRewriteManifests.java:
##########
@@ -251,13 +251,15 @@ private void performRewrite(List<ManifestFile> currentManifests) {
}
}
- private void validateDeletedManifests(Set<ManifestFile> currentManifests) {
+ private void validateDeletedManifests(Set<ManifestFile> currentManifests, long currentSnapshotId) {
// directly deleted manifests must be still present in the current snapshot
deletedManifests.stream()
.filter(manifest -> !currentManifests.contains(manifest))
.findAny()
.ifPresent(manifest -> {
- throw new ValidationException("Manifest is missing: %s", manifest.path());
+ throw new ValidationException("Cannot apply RewriteManifests result, " +
Review Comment:
@RussellSpitzer thx for the guide, I have already remove the snapshot id and use the msg u suggested.
--
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 #4612: Core:Optimize Error msg in validateDeletedManifests (fix #3466)
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #4612:
URL: https://github.com/apache/iceberg/pull/4612#discussion_r857160120
##########
core/src/main/java/org/apache/iceberg/BaseRewriteManifests.java:
##########
@@ -257,7 +257,10 @@ private void validateDeletedManifests(Set<ManifestFile> currentManifests) {
.filter(manifest -> !currentManifests.contains(manifest))
.findAny()
.ifPresent(manifest -> {
- throw new ValidationException("Manifest is missing: %s", manifest.path());
+ throw new ValidationException("Cannot commit RewriteManifests; " +
+ "manifests that existed at the beginning of this rewrite have already been removed by another " +
+ "operation. Manifest %s, which would be replaced by this operation, has already been removed.",
+ manifest.path());
Review Comment:
I think this is too long to be an effective error message. I think your intent is to tell the end user that something changed. But there isn't anything that user can do besides retry the operation. I think the short message works fine, but you could also extend it to "Manifest is missing: %s (table changed concurrently)". That's more clear without being confusing or overly long.
--
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] flyrain commented on a diff in pull request #4612: Core:Optimize Error msg in validateDeletedManifests (fix #3466)
Posted by GitBox <gi...@apache.org>.
flyrain commented on code in PR #4612:
URL: https://github.com/apache/iceberg/pull/4612#discussion_r856565023
##########
core/src/main/java/org/apache/iceberg/BaseRewriteManifests.java:
##########
@@ -251,13 +251,15 @@ private void performRewrite(List<ManifestFile> currentManifests) {
}
}
- private void validateDeletedManifests(Set<ManifestFile> currentManifests) {
+ private void validateDeletedManifests(Set<ManifestFile> currentManifests, long currentSnapshotId) {
// directly deleted manifests must be still present in the current snapshot
deletedManifests.stream()
.filter(manifest -> !currentManifests.contains(manifest))
.findAny()
.ifPresent(manifest -> {
- throw new ValidationException("Manifest is missing: %s", manifest.path());
+ throw new ValidationException("Cannot apply RewriteManifests result, " +
Review Comment:
Agreed with @RussellSpitzer. The action `RewriteManifests` doesn't need users to input any snapshot id. A user couldn't do much with the 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
Re: [PR] Core:Optimize Error msg in validateDeletedManifests (fix #3466) [iceberg]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #4612:
URL: https://github.com/apache/iceberg/pull/4612#issuecomment-2292491799
This pull request has been closed due to lack of activity. This is not a judgement on the merit of the PR in any way. It is just a way of keeping the PR queue manageable. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.
--
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
Re: [PR] Core:Optimize Error msg in validateDeletedManifests (fix #3466) [iceberg]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #4612:
URL: https://github.com/apache/iceberg/pull/4612#issuecomment-2274584692
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions.
--
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] kyle-cx commented on a diff in pull request #4612: Core:Optimize Error msg in validateDeletedManifests (fix #3466)
Posted by GitBox <gi...@apache.org>.
kyle-cx commented on code in PR #4612:
URL: https://github.com/apache/iceberg/pull/4612#discussion_r857456460
##########
core/src/main/java/org/apache/iceberg/BaseRewriteManifests.java:
##########
@@ -257,7 +257,10 @@ private void validateDeletedManifests(Set<ManifestFile> currentManifests) {
.filter(manifest -> !currentManifests.contains(manifest))
.findAny()
.ifPresent(manifest -> {
- throw new ValidationException("Manifest is missing: %s", manifest.path());
+ throw new ValidationException("Cannot commit RewriteManifests; " +
+ "manifests that existed at the beginning of this rewrite have already been removed by another " +
+ "operation. Manifest %s, which would be replaced by this operation, has already been removed.",
+ manifest.path());
Review Comment:
here is the new err msg, looks simple and clear 😄, thx for the suggestion @rdblue @RussellSpitzer
"Manifest is missing: %s (caused by table changed concurrently)"
--
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 a diff in pull request #4612: Core:Optimize Error msg in validateDeletedManifests (fix #3466)
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on code in PR #4612:
URL: https://github.com/apache/iceberg/pull/4612#discussion_r856264100
##########
core/src/main/java/org/apache/iceberg/BaseRewriteManifests.java:
##########
@@ -251,13 +251,15 @@ private void performRewrite(List<ManifestFile> currentManifests) {
}
}
- private void validateDeletedManifests(Set<ManifestFile> currentManifests) {
+ private void validateDeletedManifests(Set<ManifestFile> currentManifests, long currentSnapshotId) {
// directly deleted manifests must be still present in the current snapshot
deletedManifests.stream()
.filter(manifest -> !currentManifests.contains(manifest))
.findAny()
.ifPresent(manifest -> {
- throw new ValidationException("Manifest is missing: %s", manifest.path());
+ throw new ValidationException("Cannot apply RewriteManifests result, " +
Review Comment:
I think in this message we want to make clear that this is not a corrupt table issue and that it is most likely the result of a concurrent manifest operation. Maybe something like
"Cannot commit RewriteManifests; manifests that existed at the beginning of this rewrite have already been removed by another operation. Manifest %s, which would be replaced by this operation, has already been removed."?
I'm not sure the snapshot ID really helps unless you also pass the snapshot that the operation started on, but even with that information I'm not sure a User can do much.
--
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 a diff in pull request #4612: Core:Optimize Error msg in validateDeletedManifests (fix #3466)
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on code in PR #4612:
URL: https://github.com/apache/iceberg/pull/4612#discussion_r857711524
##########
core/src/main/java/org/apache/iceberg/BaseRewriteManifests.java:
##########
@@ -257,7 +257,10 @@ private void validateDeletedManifests(Set<ManifestFile> currentManifests) {
.filter(manifest -> !currentManifests.contains(manifest))
.findAny()
.ifPresent(manifest -> {
- throw new ValidationException("Manifest is missing: %s", manifest.path());
+ throw new ValidationException("Cannot commit RewriteManifests; " +
+ "manifests that existed at the beginning of this rewrite have already been removed by another " +
+ "operation. Manifest %s, which would be replaced by this operation, has already been removed.",
+ manifest.path());
Review Comment:
I think "Manifest is missing" is a very confusing message. From my talks with folks who have hit this as well as other Iceberg dev's, the gut reaction to this message is that the table has become corrupted. After all why would "manifest is missing" be an issue for an operation which is removing manifests? Having a missing manifest also generally means that the table is no longer stable so users are on high alert.
To further confound things, the manifest probably still exists and a user can find it, which leads them to ask "Why would Iceberg think the file is missing when it is clearly on disk?". Users often let me know in conjunction that Iceberg says a manifest is missing but they can see it on disk
In my opinion a good error message has to explain what went wrong, why it went wrong, and how to respond (if such a response is possible and non-obvious).
At the moment the error message at best explains the "why it went wrong" but without enough context to really let the user even know what part of the rewrite actually broke.
If I was going to order information here the most important concept to get across is
1. "What" - The commit has failed (not writing, reading or deleting manifests)
2. "Why" - The change we prepared no longer cleanly applies because a manifest we thought existed is no longer current
3. "Remediation" - Retry the operation if desired
So if we want to trim it down to the essentials
```
"Cannot commit, table has been concurrently modified and manifest %s is no longer part of the table"
```
Although even knowing which manifest is missing is not even that valid so we could remove that too.
I actually have issues with most of our Validation messages in MergingSnapshotProducer for the similar reason. Users are often confused by "Found conflicting files that can contain records matching true". But we can save that for future PR's
--
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
Re: [PR] Core:Optimize Error msg in validateDeletedManifests (fix #3466) [iceberg]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #4612: Core:Optimize Error msg in validateDeletedManifests (fix #3466)
URL: https://github.com/apache/iceberg/pull/4612
--
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