add sequenceName and currentCheckPoint for backwards compatibility#8864
add sequenceName and currentCheckPoint for backwards compatibility#8864fjy merged 10 commits intoapache:masterfrom
Conversation
| return checkpointMetadata; | ||
| } | ||
|
|
||
| // For backwards compatibility |
There was a problem hiding this comment.
Can add a unit test for the serialization/deserialization between old and new DTOs
There was a problem hiding this comment.
yeah, was trying to add a serde test for CheckPointDataSourceMetadataAction, but wasn't easy to construct an object because of abstract SeekableStreamDataSourceMetadata in this package.
There was a problem hiding this comment.
One option is to manually create a JSON string and then deserialize it to a different version. If you don't want to manually create the JSON, then one option is to add a class in the test file that is the old DTO.
There was a problem hiding this comment.
Added a serde test for kafka
|
This pull request introduces 1 alert when merging eb9aa74 into ce4ee42 - view on LGTM.com new alerts:
|
|
This pull request introduces 1 alert when merging 7388323 into ce4ee42 - view on LGTM.com new alerts:
|
| } | ||
|
|
||
| @Override | ||
| public boolean equals(Object o) |
There was a problem hiding this comment.
Since you have an override for equals, you'll need to also override hashCode
FYI, the SonarLint intellij plugin will give you a warning if you don't override both.
While typing this, I just noticed LGTM flagged the same issue.
| // For backwards compatibility | ||
| @Deprecated | ||
| @JsonProperty | ||
| public SeekableStreamDataSourceMetadata getCurrentCheckPoint() |
There was a problem hiding this comment.
Since this is just for Jackson, you can make it private and then get rid of the @Deprecated if you want
| */ | ||
| @Deprecated | ||
| @JsonProperty("sequenceName") | ||
| public String getBaseSequenceName() |
gianm
left a comment
gianm
left a comment
There was a problem hiding this comment.
Mostly looks good, just some comments about the comments.
| } | ||
|
|
||
| /** | ||
| * Returning a dummy value so the objects from older versions still work |
There was a problem hiding this comment.
How would returning a dummy value make objects from older versions work with the current version? Modifying the serialization code now won't change what older versions do.
There was a problem hiding this comment.
This method is to add the missing property (sequenceName) in newer serialized JSON. So the task fails on older middlemanager since it requires this property, I think it doesn't matter what value is returned here, as long as the property is present since the value is not really used in any production code. But i guess the javadoc can be updated to say it's for rolling updates.
| /** | ||
| * Returning a dummy value so the objects from older versions still work | ||
| * with current version | ||
| * TODO : this should be removed in the next release |
There was a problem hiding this comment.
I don't think "next release" is right, maybe a few releases down the road. How about: it should be removed when we don't need rolling-update compatibility with version X anymore (please fill in X with the right value).
There was a problem hiding this comment.
sounds good, i guess X should be 0.15 or earlier
|
This needs a merge from master to address the CI issue. Could you please do that and adjust the comments? |
…pache#8864) * add sequenceName and currentCheckPoint for backwards compatibility * Add serde unit test in kafka * fix checkstyle * add hashcode * update javadoc
…pache#8864) * add sequenceName and currentCheckPoint for backwards compatibility * Add serde unit test in kafka * fix checkstyle * add hashcode * update javadoc
5 participants
Fixes #8837
Description
Added
sequenceNameandcurrentCheckPointtoCheckPointDataSourceMetadataActionfor older version objects (<0.15) to work with newer version (<0.16).This PR has: