New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[FLINK-5602] migration namespace serializer #3200
[FLINK-5602] migration namespace serializer #3200
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just looked at the last commit with the artificial namespace serializer. Pretty cool idea of fixing this!
It looks good to me, I only had minor inline comments/questions.
import java.io.IOException; | ||
import java.io.Serializable; | ||
|
||
public class MigrationNamespaceSerializerProxy extends TypeSerializer<Serializable> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would add short class level JavaDoc.
|
||
public class MigrationNamespaceSerializerProxy extends TypeSerializer<Serializable> { | ||
|
||
public static MigrationNamespaceSerializerProxy INSTANCE = new MigrationNamespaceSerializerProxy(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make final?
|
||
@Override | ||
public TypeSerializer<Serializable> duplicate() { | ||
return new MigrationNamespaceSerializerProxy(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Return INSTANCE
?
&& stateSerializer.isCompatibleWith(other.stateSerializer); | ||
return (stateSerializer.isCompatibleWith(other.stateSerializer)) && | ||
(namespaceSerializer.isCompatibleWith(other.namespaceSerializer) | ||
|| other.namespaceSerializer instanceof MigrationNamespaceSerializerProxy); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would add a short comment that it's important to check for the MigrationNamespaceSerializerProxy
. It could even warrant a test. I can see how someone removes this line in the future.
49d374f
to
3133fd0
Compare
Thanks for the review @uce ! I changed my PR according to your suggestions (except for only throwing an unsupported op exception on the duplicate function). |
OK thanks! Makes sense with the UnsupportedOpException. I thought that duplicate is called somewhere maybe. I'll merge this as soon as Travis gives the green light. |
3133fd0
to
45af1c2
Compare
Verified that this fixes the issue. Thanks! Merging this with together with #3198. |
This closes apache#3200. This closes apache#3198.
This PR fixes FLINK-5602. We introduce an artificial namespace serializer instead of
null
so that checkpoints can run even before the user states are registered.This sits on top of PR #3198 .