You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@streampark.apache.org by GitBox <gi...@apache.org> on 2022/10/14 06:23:03 UTC
[GitHub] [incubator-streampark] youyangkou opened a new pull request, #1839: [Bug] fixed the ignore restored option bug
youyangkou opened a new pull request, #1839:
URL: https://github.com/apache/incubator-streampark/pull/1839
<!--
Thank you for contributing to StreamPark! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
## Contribution Checklist
- If this is your first time, please read our contributor guidelines: [Submit Code](https://streampark.apache.org/community/submit_guide/submit_code).
- Make sure that the pull request corresponds to a [GITHUB issue](https://github.com/apache/streampark/issues).
- Name the pull request in the form "[Feature] Title of the pull request", where *Feature* can be replaced by `Hotfix`, `Bug`, etc.
- Fill out the template below to describe the changes contributed by the pull request. That will give reviewers the context they need to do the review.
- If the PR is unfinished, add `[WIP]` in your PR title, e.g., `[WIP][Feature] Title of the pull request`.
-->
## What changes were proposed in this pull request
Issue Number: close [1838](https://github.com/apache/incubator-streampark/issues/1838)<!-- REMOVE this line if no issue to close -->
<!--(For example: This pull request proposed to add checkstyle plugin).-->
## Brief change log
<!--*(for example:)*
- *Add maven-checkstyle-plugin to root pom.xml*
-->
## Verifying this change
<!--*(Please pick either of the following options)*-->
This change is a trivial rework / code cleanup without any test coverage.
*(or)*
This change is already covered by existing tests, such as *(please describe tests)*.
*(or)*
This change added tests and can be verified as follows:
<!--*(example:)*
- *Added integration tests for end-to-end.*
- *Added *Test to verify the change.*
- *Manually verified the change by testing locally.* -->
## Does this pull request potentially affect one of the following parts
- Dependencies (does it add or upgrade a dependency): (yes / no)
--
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@streampark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-streampark] 1996fanrui commented on pull request #1839: [Bug] fixed the ignore restored option bug
Posted by GitBox <gi...@apache.org>.
1996fanrui commented on PR #1839:
URL: https://github.com/apache/incubator-streampark/pull/1839#issuecomment-1279204850
Do you run your job in the yarn application mode?
After read related code, I'm sure the `savepointRestoreSettings` doesn't take effect in the yarn application mode.
--
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@streampark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-streampark] 1996fanrui commented on pull request #1839: [Bug] fixed the ignore restored option bug
Posted by GitBox <gi...@apache.org>.
1996fanrui commented on PR #1839:
URL: https://github.com/apache/incubator-streampark/pull/1839#issuecomment-1279210406
> Yes, I tested the application and session modes of k8s and yarn and this parameter does not work
Do you mean it doesn't take affect in the k8s session mode?
--
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@streampark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-streampark] 1996fanrui commented on pull request #1839: [Bug] fixed the ignore restored option bug
Posted by GitBox <gi...@apache.org>.
1996fanrui commented on PR #1839:
URL: https://github.com/apache/incubator-streampark/pull/1839#issuecomment-1279192849
Hi @youyangkou , thanks for your contribution. Do you run job in standalone or yarn?
I see the `submitRequest.savepointRestoreSettings` has been set to the PackagedProgram. I will analyze why it doesn't work tomorrow.
```
val packageProgram = PackagedProgram
.newBuilder
.setSavepointRestoreSettings(submitRequest.savepointRestoreSettings)
.setJarFile(jarFile)
.setEntryPointClassName(flinkConfig.getOptional(ApplicationConfiguration.APPLICATION_MAIN_CLASS).get())
.setArguments(
flinkConfig
.getOptional(ApplicationConfiguration.APPLICATION_ARGS)
.orElse(Lists.newArrayList()): _*
).build()
```
--
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@streampark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-streampark] 1996fanrui commented on a diff in pull request #1839: [Bug] fixed the ignore restored option bug
Posted by GitBox <gi...@apache.org>.
1996fanrui commented on code in PR #1839:
URL: https://github.com/apache/incubator-streampark/pull/1839#discussion_r996237973
##########
streampark-flink/streampark-flink-submit/streampark-flink-submit-core/src/main/scala/org/apache/streampark/flink/submit/trait/FlinkSubmitTrait.scala:
##########
@@ -105,6 +105,9 @@ trait FlinkSubmitTrait extends Logger {
val retainedOption = CheckpointingOptions.MAX_RETAINED_CHECKPOINTS
flinkConfig.set(retainedOption, flinkDefaultConfiguration.get(retainedOption))
+ //set savepoint.ignore-unclaimed-state parameter
+ flinkConfig.setBoolean(SavepointConfigOptions.SAVEPOINT_IGNORE_UNCLAIMED_STATE, submitRequest.savepointRestoreSettings.allowNonRestoredState())
Review Comment:
Hi @wolfboys , please help double check this solution in your free time, thanks~
--
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@streampark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-streampark] youyangkou commented on pull request #1839: [Bug] fixed the ignore restored option bug
Posted by GitBox <gi...@apache.org>.
youyangkou commented on PR #1839:
URL: https://github.com/apache/incubator-streampark/pull/1839#issuecomment-1279209239
> Yes, I tested the application and session modes of k8s and yarn and this parameter does not work
--
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@streampark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-streampark] wolfboys merged pull request #1839: [Bug] fixed the ignore restored option bug
Posted by GitBox <gi...@apache.org>.
wolfboys merged PR #1839:
URL: https://github.com/apache/incubator-streampark/pull/1839
--
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@streampark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-streampark] wolfboys commented on a diff in pull request #1839: [Bug] fixed the ignore restored option bug
Posted by GitBox <gi...@apache.org>.
wolfboys commented on code in PR #1839:
URL: https://github.com/apache/incubator-streampark/pull/1839#discussion_r997049181
##########
streampark-flink/streampark-flink-submit/streampark-flink-submit-core/src/main/scala/org/apache/streampark/flink/submit/trait/FlinkSubmitTrait.scala:
##########
@@ -105,6 +105,9 @@ trait FlinkSubmitTrait extends Logger {
val retainedOption = CheckpointingOptions.MAX_RETAINED_CHECKPOINTS
flinkConfig.set(retainedOption, flinkDefaultConfiguration.get(retainedOption))
+ //set savepoint.ignore-unclaimed-state parameter
+ flinkConfig.setBoolean(SavepointConfigOptions.SAVEPOINT_IGNORE_UNCLAIMED_STATE, submitRequest.savepointRestoreSettings.allowNonRestoredState())
Review Comment:
> Hi @wolfboys , please help double check this solution in your free time, thanks~
👌 I'll check it leater
--
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@streampark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-streampark] 1996fanrui commented on a diff in pull request #1839: [Bug] fixed the ignore restored option bug
Posted by GitBox <gi...@apache.org>.
1996fanrui commented on code in PR #1839:
URL: https://github.com/apache/incubator-streampark/pull/1839#discussion_r996237931
##########
streampark-flink/streampark-flink-submit/streampark-flink-submit-core/src/main/scala/org/apache/streampark/flink/submit/trait/FlinkSubmitTrait.scala:
##########
@@ -105,6 +105,9 @@ trait FlinkSubmitTrait extends Logger {
val retainedOption = CheckpointingOptions.MAX_RETAINED_CHECKPOINTS
flinkConfig.set(retainedOption, flinkDefaultConfiguration.get(retainedOption))
+ //set savepoint.ignore-unclaimed-state parameter
+ flinkConfig.setBoolean(SavepointConfigOptions.SAVEPOINT_IGNORE_UNCLAIMED_STATE, submitRequest.savepointRestoreSettings.allowNonRestoredState())
Review Comment:
As I understand, `SavepointConfigOptions.SAVEPOINT_PATH` and `SavepointConfigOptions.SAVEPOINT_IGNORE_UNCLAIMED_STATE` can replace the `submitRequest.savepointRestoreSettings`. And the `submitRequest.savepointRestoreSettings` isn't called in application mode, so it's a bug.
So I suggest remove the `submitRequest.savepointRestoreSettings`.
--
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@streampark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-streampark] youyangkou commented on pull request #1839: [Bug] fixed the ignore restored option bug
Posted by GitBox <gi...@apache.org>.
youyangkou commented on PR #1839:
URL: https://github.com/apache/incubator-streampark/pull/1839#issuecomment-1288139811
> hi youyang:
>
> Thank you for your contribution! the bug does exist, and your current commit fixed it, but the code can be further improved:
>
> 1. delete 163 lines of code in FlinkSubmitTrait:
> `.setSavepointRestoreSettings(submitRequest.savepointRestoreSettings)`
> 2. `savepointRestoreSettings` need improvement
>
> before:
>
> ```
> lazy val savepointRestoreSettings: SavepointRestoreSettings = {
> lazy val allowNonRestoredState = Try(extraParameter.get(SavepointConfigOptions.SAVEPOINT_IGNORE_UNCLAIMED_STATE.key).toString.toBoolean).getOrElse(false)
> savePoint match {
> case sp if Try(sp.isEmpty).getOrElse(true) => SavepointRestoreSettings.none
> case sp => SavepointRestoreSettings.forPath(sp, allowNonRestoredState)
> }
> }
> ```
>
> after:
>
> ```
> lazy val allowNonRestoredState = Try(extraParameter.get(SavepointConfigOptions.SAVEPOINT_IGNORE_UNCLAIMED_STATE.key).toString.toBoolean).getOrElse(false)
> ```
>
> 3. add code line 101 in FlinkSubmitTrait:
>
> ```
> .safeSet(SavepointConfigOptions.SAVEPOINT_IGNORE_UNCLAIMED_STATE, submitRequest.allowNonRestoredState)
> ```
Thanks for your review, addressed.
--
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@streampark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org