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/11/16 12:27:44 UTC
[GitHub] [iceberg] xloya opened a new pull request #3563: Spark: Rewrite if no commit succeeded throw commit failed exception
xloya opened a new pull request #3563:
URL: https://github.com/apache/iceberg/pull/3563
If parital commit was enabled but no commits succeeded at the end, we should throw exception and have no need to collect result.
If no exception is thrown, the Spark program will consider the entire compaction process to be successful and return the final state of success.
cc @aokolnychyi @rdblue What do you think about it? thx!
--
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] xloya commented on a change in pull request #3563: Spark: Rewrite if no commit succeeded throw no commit succeed exception
Posted by GitBox <gi...@apache.org>.
xloya commented on a change in pull request #3563:
URL: https://github.com/apache/iceberg/pull/3563#discussion_r751875077
##########
File path: spark/v3.2/spark/src/main/java/org/apache/iceberg/spark/actions/BaseRewriteDataFilesSparkAction.java
##########
@@ -327,6 +328,7 @@ private Result doExecuteWithPartialProgress(RewriteExecutionContext ctx, Stream<
LOG.error("{} is true but no rewrite commits succeeded. Check the logs to determine why the individual " +
"commits failed. If this is persistent it may help to increase {} which will break the rewrite operation " +
"into smaller commits.", PARTIAL_PROGRESS_ENABLED, PARTIAL_PROGRESS_MAX_COMMITS);
+ throw new NoCommitSucceedException("No rewrite commits succeeded after max commits");
Review comment:
@rdblue I see that we have plans to rewrite delete files, if we support partial commit for rewrite delete files, it also will have a no-commit-succeed scenario. So i think maybe it makes sense to create a new exception.
--
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] xloya commented on a change in pull request #3563: Spark: Rewrite if no commit succeeded throw no commit succeed exception
Posted by GitBox <gi...@apache.org>.
xloya commented on a change in pull request #3563:
URL: https://github.com/apache/iceberg/pull/3563#discussion_r751873641
##########
File path: api/src/main/java/org/apache/iceberg/exceptions/NoCommitSucceedException.java
##########
@@ -0,0 +1,41 @@
+/*
+ * 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.exceptions;
+
+import com.google.errorprone.annotations.FormatMethod;
+
+/**
+ * Exception raised when no commit succeed.
+ * <p>
+ * For example, this is thrown when rewriting data files partial commit is enabled,
+ * but no commit succeed at the end.
+ */
+public class NoCommitSucceedException extends RuntimeException {
Review comment:
@rdblue sounds great! I'll I will define this exception class in `RewriteDataFiles`.
--
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] kbendick commented on a change in pull request #3563: Spark: Rewrite if no commit succeeded throw no commit succeed exception
Posted by GitBox <gi...@apache.org>.
kbendick commented on a change in pull request #3563:
URL: https://github.com/apache/iceberg/pull/3563#discussion_r752906737
##########
File path: api/src/main/java/org/apache/iceberg/actions/RewriteDataFiles.java
##########
@@ -88,6 +89,15 @@
String USE_STARTING_SEQUENCE_NUMBER = "use-starting-sequence-number";
boolean USE_STARTING_SEQUENCE_NUMBER_DEFAULT = true;
+ /**
+ * If we make {@link #PARTIAL_PROGRESS_ENABLED} to be true, for some reasons, the all commits may not be successful
+ * in the end. If we do rewrite in an isolated job, wo may want to fail the entire job. But if we do rewrite after
+ * writing, may not want to fail the entire job. This setting determines whether the entire rewrite process will fail
+ * if no commits succeeded in the end. This setting has no effect if partial progress is disabled.
+ */
+ String IGNORE_NO_SUCCESSFUL_COMMIT_ENABLED = "ignore-no-successful-commit.enabled";
Review comment:
Nit: is the name of this config value clear to others?
I would maybe consider something along the lines of `partial-progress.ignore-all-commits-failed.enabled`.
First, since there are other configs prefixed with `partial-progress.`, I would follow that convention. And then whichever exception is used in the end, I would use that in the name. But the big thing is prefacing with `partial-progress` as that’s already in the file.
--
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] xloya commented on a change in pull request #3563: Spark: Rewrite if no commit succeeded throw commit failed exception
Posted by GitBox <gi...@apache.org>.
xloya commented on a change in pull request #3563:
URL: https://github.com/apache/iceberg/pull/3563#discussion_r750827933
##########
File path: spark/v3.2/spark/src/main/java/org/apache/iceberg/spark/actions/BaseRewriteDataFilesSparkAction.java
##########
@@ -327,6 +327,7 @@ private Result doExecuteWithPartialProgress(RewriteExecutionContext ctx, Stream<
LOG.error("{} is true but no rewrite commits succeeded. Check the logs to determine why the individual " +
"commits failed. If this is persistent it may help to increase {} which will break the rewrite operation " +
"into smaller commits.", PARTIAL_PROGRESS_ENABLED, PARTIAL_PROGRESS_MAX_COMMITS);
+ throw new CommitFailedException("No rewrite commits succeeded after max commits");
Review comment:
Hey, @RussellSpitzer thanks for your review! I got your point, I think we can throw a new NoCommitSucceedException which extends RuntimeException here. What do you think 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.
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 change in pull request #3563: Spark: Rewrite if no commit succeeded throw no commit succeed exception
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on a change in pull request #3563:
URL: https://github.com/apache/iceberg/pull/3563#discussion_r751525845
##########
File path: spark/v3.2/spark/src/main/java/org/apache/iceberg/spark/actions/BaseRewriteDataFilesSparkAction.java
##########
@@ -327,6 +328,7 @@ private Result doExecuteWithPartialProgress(RewriteExecutionContext ctx, Stream<
LOG.error("{} is true but no rewrite commits succeeded. Check the logs to determine why the individual " +
"commits failed. If this is persistent it may help to increase {} which will break the rewrite operation " +
"into smaller commits.", PARTIAL_PROGRESS_ENABLED, PARTIAL_PROGRESS_MAX_COMMITS);
+ throw new NoCommitSucceedException("No rewrite commits succeeded after max commits");
Review comment:
I mentioned this above, but we won't get CommitFailedExceptions for a bunch of other cases. Like say the user lacks permission to write to storage, or there is a network error while a shuffle is occurring.
--
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] xloya commented on pull request #3563: Spark: Rewrite if no commit succeeded throw no commit succeed exception
Posted by GitBox <gi...@apache.org>.
xloya commented on pull request #3563:
URL: https://github.com/apache/iceberg/pull/3563#issuecomment-971308599
Hi @kbendick , firstly thanks for your review!
You did mention another usage scenario that I didn't think about. We generally do use an isolated spark job for compaction. So we need a spark job tells us no commit succeed by an exception to allows us to process in time without affecting query efficiency. It is also very normal to execute compaction after writing.
Can we use a compromise method, such as adding an allow-no-commit-succeed configuration that defaults to true, and then customize this parameter in a spark job to choose whether to throw an exception?
--
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] xloya commented on a change in pull request #3563: Spark: Rewrite if no commit succeeded throw no commit succeed exception
Posted by GitBox <gi...@apache.org>.
xloya commented on a change in pull request #3563:
URL: https://github.com/apache/iceberg/pull/3563#discussion_r752918062
##########
File path: api/src/main/java/org/apache/iceberg/actions/RewriteDataFiles.java
##########
@@ -88,6 +89,15 @@
String USE_STARTING_SEQUENCE_NUMBER = "use-starting-sequence-number";
boolean USE_STARTING_SEQUENCE_NUMBER_DEFAULT = true;
+ /**
+ * If we make {@link #PARTIAL_PROGRESS_ENABLED} to be true, for some reasons, the all commits may not be successful
+ * in the end. If we do rewrite in an isolated job, wo may want to fail the entire job. But if we do rewrite after
+ * writing, may not want to fail the entire job. This setting determines whether the entire rewrite process will fail
+ * if no commits succeeded in the end. This setting has no effect if partial progress is disabled.
+ */
+ String IGNORE_NO_SUCCESSFUL_COMMIT_ENABLED = "ignore-no-successful-commit.enabled";
Review comment:
Sounds great! I will add a prefix name of the setting, but according to what @RussellSpitzer said above, it may not be able to use `all-commits-failed`, it may just be `no-commit-succeed` because the definition of `commit-failed` may be a metadata operation failure in our project.
--
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] xloya commented on a change in pull request #3563: Spark: Rewrite if no commit succeeded throw no commit succeed exception
Posted by GitBox <gi...@apache.org>.
xloya commented on a change in pull request #3563:
URL: https://github.com/apache/iceberg/pull/3563#discussion_r751873641
##########
File path: api/src/main/java/org/apache/iceberg/exceptions/NoCommitSucceedException.java
##########
@@ -0,0 +1,41 @@
+/*
+ * 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.exceptions;
+
+import com.google.errorprone.annotations.FormatMethod;
+
+/**
+ * Exception raised when no commit succeed.
+ * <p>
+ * For example, this is thrown when rewriting data files partial commit is enabled,
+ * but no commit succeed at the end.
+ */
+public class NoCommitSucceedException extends RuntimeException {
Review comment:
@rdblue sounds great! I will define this exception class in `RewriteDataFiles`.
--
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 change in pull request #3563: Spark: Rewrite if no commit succeeded throw commit failed exception
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on a change in pull request #3563:
URL: https://github.com/apache/iceberg/pull/3563#discussion_r750501106
##########
File path: spark/v3.2/spark/src/main/java/org/apache/iceberg/spark/actions/BaseRewriteDataFilesSparkAction.java
##########
@@ -327,6 +327,7 @@ private Result doExecuteWithPartialProgress(RewriteExecutionContext ctx, Stream<
LOG.error("{} is true but no rewrite commits succeeded. Check the logs to determine why the individual " +
"commits failed. If this is persistent it may help to increase {} which will break the rewrite operation " +
"into smaller commits.", PARTIAL_PROGRESS_ENABLED, PARTIAL_PROGRESS_MAX_COMMITS);
+ throw new CommitFailedException("No rewrite commits succeeded after max commits");
Review comment:
The issue here is that it may not have been a commit failed exception, it could have been a data file permission issue, out of disk space issue, or any other host of problems. So we don't actually know if there was a commit failure, we just know that none of the commits succeeded. I think if you wanted we could still throw an error here but I don't believe it should be a commit failed exception.
--
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] xloya edited a comment on pull request #3563: Spark: Rewrite if no commit succeeded throw no commit succeed exception
Posted by GitBox <gi...@apache.org>.
xloya edited a comment on pull request #3563:
URL: https://github.com/apache/iceberg/pull/3563#issuecomment-971308599
Hi @kbendick , firstly thanks for your review!
You did mention another usage scenario that I didn't think about. We generally do use an isolated spark job for compaction. So we need a spark job tells us no commit succeeded by an exception to allows us to process in time without affecting query efficiency. It is also very normal to execute compaction after writing in the same one spark job.
Can we use a compromise method, such as adding an `allow-no-commit-succeed` configuration that defaults to `true`, and then customize it in a spark job to choose whether to throw an exception?
--
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 change in pull request #3563: Spark: Rewrite if no commit succeeded throw no commit succeed exception
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #3563:
URL: https://github.com/apache/iceberg/pull/3563#discussion_r751516529
##########
File path: api/src/main/java/org/apache/iceberg/exceptions/NoCommitSucceedException.java
##########
@@ -0,0 +1,41 @@
+/*
+ * 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.exceptions;
+
+import com.google.errorprone.annotations.FormatMethod;
+
+/**
+ * Exception raised when no commit succeed.
+ * <p>
+ * For example, this is thrown when rewriting data files partial commit is enabled,
+ * but no commit succeed at the end.
+ */
+public class NoCommitSucceedException extends RuntimeException {
Review comment:
I'd rather not add this. I think it is going to cause confusion because we would have both `CommitFailedException` and `NoCommitSucceededException`. If you want to define this within the action, like `RewriteDataFiles.NoSuccessfulCommit` then that's okay. But we shouldn't add it to the general 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.
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] xloya edited a comment on pull request #3563: Spark: Rewrite if no commit succeeded throw no commit succeed exception
Posted by GitBox <gi...@apache.org>.
xloya edited a comment on pull request #3563:
URL: https://github.com/apache/iceberg/pull/3563#issuecomment-971308599
Hi @kbendick , firstly thanks for your review!
You did mention another usage scenario that I didn't think about. We generally do use an isolated spark job for compaction. So we need a spark job tells us no commit succeeded by an exception to allows us to process in time without affecting query efficiency. It is also very normal to execute compaction after writing.
Can we use a compromise method, such as adding an `allow-no-commit-succeed` configuration that defaults to `true`, and then customize this parameter in a spark job to choose whether to throw an exception?
--
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] xloya edited a comment on pull request #3563: Spark: Rewrite if no commit succeeded throw no commit succeed exception
Posted by GitBox <gi...@apache.org>.
xloya edited a comment on pull request #3563:
URL: https://github.com/apache/iceberg/pull/3563#issuecomment-971308599
Hi @kbendick , firstly thanks for your review!
You did mention another usage scenario that I didn't think about. We generally do use an isolated spark job for compaction. So we need a spark job tells us no commit succeed by an exception to allows us to process in time without affecting query efficiency. It is also very normal to execute compaction after writing.
Can we use a compromise method, such as adding an allow-no-commit-succeed configuration that defaults to true, and then customize this parameter in a spark job to choose whether to throw an exception?
--
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 change in pull request #3563: Spark: Rewrite if no commit succeeded throw no commit succeed exception
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #3563:
URL: https://github.com/apache/iceberg/pull/3563#discussion_r751629990
##########
File path: spark/v3.2/spark/src/main/java/org/apache/iceberg/spark/actions/BaseRewriteDataFilesSparkAction.java
##########
@@ -327,6 +328,7 @@ private Result doExecuteWithPartialProgress(RewriteExecutionContext ctx, Stream<
LOG.error("{} is true but no rewrite commits succeeded. Check the logs to determine why the individual " +
"commits failed. If this is persistent it may help to increase {} which will break the rewrite operation " +
"into smaller commits.", PARTIAL_PROGRESS_ENABLED, PARTIAL_PROGRESS_MAX_COMMITS);
+ throw new NoCommitSucceedException("No rewrite commits succeeded after max commits");
Review comment:
Sounds good. Commit failed is not a good idea then. What else could we throw instead of creating a new exception?
--
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] xloya edited a comment on pull request #3563: Spark: Rewrite if no commit succeeded throw no commit succeed exception
Posted by GitBox <gi...@apache.org>.
xloya edited a comment on pull request #3563:
URL: https://github.com/apache/iceberg/pull/3563#issuecomment-971308599
--
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] xloya commented on a change in pull request #3563: Spark: Rewrite if no commit succeeded throw no commit succeed exception
Posted by GitBox <gi...@apache.org>.
xloya commented on a change in pull request #3563:
URL: https://github.com/apache/iceberg/pull/3563#discussion_r752918062
##########
File path: api/src/main/java/org/apache/iceberg/actions/RewriteDataFiles.java
##########
@@ -88,6 +89,15 @@
String USE_STARTING_SEQUENCE_NUMBER = "use-starting-sequence-number";
boolean USE_STARTING_SEQUENCE_NUMBER_DEFAULT = true;
+ /**
+ * If we make {@link #PARTIAL_PROGRESS_ENABLED} to be true, for some reasons, the all commits may not be successful
+ * in the end. If we do rewrite in an isolated job, wo may want to fail the entire job. But if we do rewrite after
+ * writing, may not want to fail the entire job. This setting determines whether the entire rewrite process will fail
+ * if no commits succeeded in the end. This setting has no effect if partial progress is disabled.
+ */
+ String IGNORE_NO_SUCCESSFUL_COMMIT_ENABLED = "ignore-no-successful-commit.enabled";
Review comment:
Sounds great! I will add a prefix name of the setting, but according to what @RussellSpitzer said above, it may not be able to use `all-commits-failed`, it may just be like `no-successful-commit` because the definition of `commit-failed` may be a metadata operation failure in our project.
--
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] xloya commented on pull request #3563: Spark: Rewrite if no commit succeeded throw no commit succeed exception
Posted by GitBox <gi...@apache.org>.
xloya commented on pull request #3563:
URL: https://github.com/apache/iceberg/pull/3563#issuecomment-973797484
Now supports rewriting whether ignore the no-commit-succeed situation when the partial commit is true, which is ignored by default. If it is not ignored, a NoCommitSuccessfulException will be thrown to interrupt the rewrite process.
--
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] kbendick commented on pull request #3563: Spark: Rewrite if no commit succeeded throw no commit succeed exception
Posted by GitBox <gi...@apache.org>.
kbendick commented on pull request #3563:
URL: https://github.com/apache/iceberg/pull/3563#issuecomment-971950077
> Hi @kbendick , firstly thanks for your review!
>
> You did mention another usage scenario that I didn't think about. We generally do use an isolated spark job for compaction. So we need a spark job tells us no commit succeeded by an exception to allows us to process in time without affecting query efficiency. It is also very normal to execute compaction after writing in the same one spark job.
>
> Can we use a compromise method, such as adding an `allow-no-commit-succeed` configuration that defaults to `true`, and then customize it in a spark job to choose whether to throw an exception?
Yeah I think adding a flag to the action would be a decent compromise. cc @RussellSpitzer on his thoughts as he's done the most work around compactions and the API for it.
My biggest concern for not just blindly throwing would be streaming ingestion, as mentioned, then followed by batch jobs that optimize as a final step.
--
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] xloya commented on a change in pull request #3563: Spark: Rewrite if no commit succeeded throw no commit succeed exception
Posted by GitBox <gi...@apache.org>.
xloya commented on a change in pull request #3563:
URL: https://github.com/apache/iceberg/pull/3563#discussion_r752918062
##########
File path: api/src/main/java/org/apache/iceberg/actions/RewriteDataFiles.java
##########
@@ -88,6 +89,15 @@
String USE_STARTING_SEQUENCE_NUMBER = "use-starting-sequence-number";
boolean USE_STARTING_SEQUENCE_NUMBER_DEFAULT = true;
+ /**
+ * If we make {@link #PARTIAL_PROGRESS_ENABLED} to be true, for some reasons, the all commits may not be successful
+ * in the end. If we do rewrite in an isolated job, wo may want to fail the entire job. But if we do rewrite after
+ * writing, may not want to fail the entire job. This setting determines whether the entire rewrite process will fail
+ * if no commits succeeded in the end. This setting has no effect if partial progress is disabled.
+ */
+ String IGNORE_NO_SUCCESSFUL_COMMIT_ENABLED = "ignore-no-successful-commit.enabled";
Review comment:
Sounds great! I will add a prefix name of the setting, but according to what @RussellSpitzer said above, it may not be able to use `all-commits-failed`, it may just be `no-commit-succeed` because the definition of `commit-failed` may be a metadata operation in our project.
--
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 change in pull request #3563: Spark: Rewrite if no commit succeeded throw no commit succeed exception
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #3563:
URL: https://github.com/apache/iceberg/pull/3563#discussion_r751517932
##########
File path: spark/v3.2/spark/src/main/java/org/apache/iceberg/spark/actions/BaseRewriteDataFilesSparkAction.java
##########
@@ -327,6 +328,7 @@ private Result doExecuteWithPartialProgress(RewriteExecutionContext ctx, Stream<
LOG.error("{} is true but no rewrite commits succeeded. Check the logs to determine why the individual " +
"commits failed. If this is persistent it may help to increase {} which will break the rewrite operation " +
"into smaller commits.", PARTIAL_PROGRESS_ENABLED, PARTIAL_PROGRESS_MAX_COMMITS);
+ throw new NoCommitSucceedException("No rewrite commits succeeded after max commits");
Review comment:
Why not throw `CommitFailedException`? That should be the underlying exception that caused all of the failures, right?
--
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 change in pull request #3563: Spark: Rewrite if no commit succeeded throw no commit succeed exception
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on a change in pull request #3563:
URL: https://github.com/apache/iceberg/pull/3563#discussion_r751666057
##########
File path: spark/v3.2/spark/src/main/java/org/apache/iceberg/spark/actions/BaseRewriteDataFilesSparkAction.java
##########
@@ -327,6 +328,7 @@ private Result doExecuteWithPartialProgress(RewriteExecutionContext ctx, Stream<
LOG.error("{} is true but no rewrite commits succeeded. Check the logs to determine why the individual " +
"commits failed. If this is persistent it may help to increase {} which will break the rewrite operation " +
"into smaller commits.", PARTIAL_PROGRESS_ENABLED, PARTIAL_PROGRESS_MAX_COMMITS);
+ throw new NoCommitSucceedException("No rewrite commits succeeded after max commits");
Review comment:
Yeah i'm not sure, I feel like this is a common pain point in asynchronous failures. Ideally I'd like to gather all unique error messages and rethrow them, but I've previously dealt with this by just keeping a "last thrown" exception and just rethrowing that.
--
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] xloya edited a comment on pull request #3563: Spark: Rewrite if no commit succeeded throw no commit succeed exception
Posted by GitBox <gi...@apache.org>.
xloya edited a comment on pull request #3563:
URL: https://github.com/apache/iceberg/pull/3563#issuecomment-971308599
Hi @kbendick , firstly thanks for your review!
You did mention another usage scenario that I didn't think about. We generally do use an isolated spark job for compaction. So we need a spark job tells us no commit succeeded by an exception to allows us to process in time without affecting query efficiency. It is also very normal to execute compaction after writing in the same one spark job.
Can we use a compromise method, such as adding an `allow-no-commit-succeed` configuration that defaults to `true`, and then customize this parameter in a spark job to choose whether to throw an exception?
--
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