You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by GitBox <gi...@apache.org> on 2022/05/17 10:12:32 UTC
[GitHub] [flink-kubernetes-operator] gyfora opened a new pull request, #218: [hotfix] Do not use nulls in status for savepoint trigger and cluster info
gyfora opened a new pull request, #218:
URL: https://github.com/apache/flink-kubernetes-operator/pull/218
Seems like there is a problem with the fabric8 patch logic and it doesn't allow us to set status fields to "null" and thus remove them.
This meant that savepoint trigger information would not be cleared correctly.
I will still investigate to see if there is a potential simpler workaround.
The problem seems to be caused by
https://github.com/fabric8io/kubernetes-client/blob/master/kubernetes-client-api/src/main/java/io/fabric8/kubernetes/client/utils/Serialization.java#L53
https://github.com/fabric8io/kubernetes-client/blob/master/kubernetes-client-api/src/main/java/io/fabric8/kubernetes/client/utils/serialization/UnmatchedFieldTypeModule.java
--
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@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink-kubernetes-operator] gyfora commented on pull request #218: [hotfix] Do not use nulls in status for savepoint trigger and cluster info
Posted by GitBox <gi...@apache.org>.
gyfora commented on PR #218:
URL: https://github.com/apache/flink-kubernetes-operator/pull/218#issuecomment-1128756928
Exactly, the path request sent to k8s should contain the null values so merge would remove those keys. Unfortunately fabric8 applies some custom plugin to the objectmapper that forces it to drop nulls (with a simple objectmapper they would be there).
So the request sent to k8s will not contian nulls so it is impossible to remove keys from the status (which is what we want when we set something to null)
--
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@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink-kubernetes-operator] Aitozi commented on pull request #218: [hotfix] Do not use nulls in status for savepoint trigger and cluster info
Posted by GitBox <gi...@apache.org>.
Aitozi commented on PR #218:
URL: https://github.com/apache/flink-kubernetes-operator/pull/218#issuecomment-1128777529
Yes, I think your solution looks good to me :) , custom serializer is a burden. +1 from my side
--
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@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink-kubernetes-operator] gyfora commented on pull request #218: [hotfix] Do not use nulls in status for savepoint trigger and cluster info
Posted by GitBox <gi...@apache.org>.
gyfora commented on PR #218:
URL: https://github.com/apache/flink-kubernetes-operator/pull/218#issuecomment-1128741701
@Aitozi it should delete the key when the value is null that is correct. But it doesn’t seem to work as it drops the null values before making the request to k8s.
you can verify by calling: Serialization.asJson(resource)
--
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@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink-kubernetes-operator] gyfora commented on pull request #218: [hotfix] Do not use nulls in status for savepoint trigger and cluster info
Posted by GitBox <gi...@apache.org>.
gyfora commented on PR #218:
URL: https://github.com/apache/flink-kubernetes-operator/pull/218#issuecomment-1128682594
@wangyang0918 do you have any alternative idea maybe? :)
--
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@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink-kubernetes-operator] Aitozi commented on pull request #218: [hotfix] Do not use nulls in status for savepoint trigger and cluster info
Posted by GitBox <gi...@apache.org>.
Aitozi commented on PR #218:
URL: https://github.com/apache/flink-kubernetes-operator/pull/218#issuecomment-1128768389
Thanks, I think I get what you mean, I tried a way to custom the serializer may be can pass the null
```
@Test
public void test() {
var application = TestUtils.buildApplicationCluster();
Serialization.jsonMapper().registerModule(new TestModule());
System.out.println(Serialization.asJson(application));
}
public class TestModule extends SimpleModule {
public TestModule() {
super("CustomTestModule", VersionUtil.versionFor(SavepointInfo.class));
addSerializer(SavepointInfo.class, new SavepointInfoSerializer());
}
}
public class SavepointInfoSerializer extends JsonSerializer<SavepointInfo> {
@Override
public void serialize(
SavepointInfo savepointInfo,
JsonGenerator jsonGenerator,
SerializerProvider serializerProvider)
throws IOException {
jsonGenerator.writeStartObject();
if (savepointInfo.getTriggerId() == null) {
jsonGenerator.writeStringField("triggerId", null);
}
}
}
```
generated:
```
"savepointInfo":{
"triggerId":null
}
```
--
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@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink-kubernetes-operator] Aitozi commented on pull request #218: [hotfix] Do not use nulls in status for savepoint trigger and cluster info
Posted by GitBox <gi...@apache.org>.
Aitozi commented on PR #218:
URL: https://github.com/apache/flink-kubernetes-operator/pull/218#issuecomment-1128732154
Does it caused by it used the `PatchType#JSON_MERGE` which will delete the key when it's value is null ?
--
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@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink-kubernetes-operator] gyfora commented on pull request #218: [hotfix] Do not use nulls in status for savepoint trigger and cluster info
Posted by GitBox <gi...@apache.org>.
gyfora commented on PR #218:
URL: https://github.com/apache/flink-kubernetes-operator/pull/218#issuecomment-1128773949
@Aitozi that would mean we need to write custom serializers instead of simply avoiding nulls (and we also rely on the implementation of the fabric8 serialization logic which makes it more unsafe)
I think avoiding nulls is simpler and feels less hacky.
--
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@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink-kubernetes-operator] Aitozi commented on pull request #218: [hotfix] Do not use nulls in status for savepoint trigger and cluster info
Posted by GitBox <gi...@apache.org>.
Aitozi commented on PR #218:
URL: https://github.com/apache/flink-kubernetes-operator/pull/218#issuecomment-1128747464
You mean it didn't keep the null value, so the patch for the field is skipped right ? @gyfora
--
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@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink-kubernetes-operator] wangyang0918 commented on pull request #218: [hotfix] Do not use nulls in status for savepoint trigger and cluster info
Posted by GitBox <gi...@apache.org>.
wangyang0918 commented on PR #218:
URL: https://github.com/apache/flink-kubernetes-operator/pull/218#issuecomment-1128787527
BTW, I have to admin that setting the value to `null` in json/yaml is not a best practice.
--
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@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink-kubernetes-operator] morhidi commented on pull request #218: [hotfix] Do not use nulls in status for savepoint trigger and cluster info
Posted by GitBox <gi...@apache.org>.
morhidi commented on PR #218:
URL: https://github.com/apache/flink-kubernetes-operator/pull/218#issuecomment-1128828889
+1 It looked scary for the first read. Happy to see an elegant solution for 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@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink-kubernetes-operator] gyfora merged pull request #218: [hotfix] Do not use nulls in status for savepoint trigger and cluster info
Posted by GitBox <gi...@apache.org>.
gyfora merged PR #218:
URL: https://github.com/apache/flink-kubernetes-operator/pull/218
--
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@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org