[FLINK-35292] Set dummy savepoint path during last-state upgrade #849
Conversation
| @@ -265,7 +274,7 @@ protected void restoreJob( | |||
| throws Exception { | |||
| Optional<String> savepointOpt = Optional.empty(); | |||
|
|
|||
| if (spec.getJob().getUpgradeMode() != UpgradeMode.STATELESS) { | |||
| if (spec.getJob().getUpgradeMode() == UpgradeMode.SAVEPOINT) { | |||
There was a problem hiding this comment.
nit: it seems appropriate to push this logic down into the deploy method. That would also eliminate usage of an optional as a method parameter (something that is debatably considered an antipattern).
There was a problem hiding this comment.
The problem is that this cannot be easily done as what savepoint is passed there will depend on where / why the deploy is called. It is for example different during initial deployments vs upgrades.
While the optional parameter is not great I would like to avoid adding more complexity to the deploy method. It may make sense to actually break up the deploy method in 3 parts, stateless/last-state/savepoint which would get rid of the optional params + the requireHaMetadata flag at the same time at the expense of more methods.
There was a problem hiding this comment.
Whether this would actually makes sense from a code readability standpoint still remains to be seen
What is the purpose of the change
Currently the operator always sets the savepoint path even if last-state (HA metadata) must be used.
This can be misleading to users as the set savepoint path normally should never take effect and can actually lead to incorrect state restored if the HA metadata is deleted by the user at the wrong moment.
To avoid this we can set an explicit dummy savepoint path which will prevent restoring from it accidentally.
Brief change log
Verifying this change
Unit tests + manual verification
Does this pull request potentially affect one of the following parts:
CustomResourceDescriptors: noDocumentation