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
[CURATOR-394] UnrecognizedPropertyException: "enabled" incompatibility #208
Conversation
…a potential UnrecognizedPropertyException in older clients that read newly serialized ServiceInstances. Added an alternate ctor to JsonInstanceSerializer with a compatibleSerializationMode option. when set to true, the new enabled field of ServiceInstance is not serialized.
private final Long new2; | ||
private final Date new3; | ||
private final URI new4; | ||
|
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.
What are these new1-new4 variables? They seem to be a new addition as part of this fix?
Also, Is there a reason that this class can't be a subclass of the OldServiceInstance?
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.
This is a test. I wanted to prove that if we add new fields in the future the same problem won't happen again.
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 also thought this is the new prod code at first glance, maybe we should rename the class to make it more test-kind of thing.
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.
Good point - I'll do that
|
||
/** | ||
* @param payloadClass used to validate payloads when deserializing | ||
*/ | ||
public JsonInstanceSerializer(Class<T> payloadClass) | ||
{ | ||
this(payloadClass, false, false); |
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.
Doesn't this approach imply that moving to Curator 2.12.x requires you to change the code base to not break backwards compatibility? Shouldn't the default be to be in compatibility mode? Then you can just upgrade to 2.12.x and everything will still work, but you need to change the code base to pick up the fix for CURATOR-275?
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.
Hmm - that's a good point.
@cammckenzie I've turned off default serialization of |
I think that this is OK. I'm just wondering what the best way forward is after this release? Ideally we don't want to have to have users opt in to pick up CURATOR-275 do we? I've never used the service discovery stuff, so I'm not sure if that's an issue. Should the next release after the one that includes this have the serialization of enabled set to true? Then we would have an upgrade path from pre 2.12.x to 2.12.x+1 (with this fix) to 2.12.x + 2? |
@cammckenzie TBH - that's why I didn't make compatibleSerializationMode the default originally. I don't have a good answer. Any idea? Maybe we just comment that we'll reverse this in a future release? |
I can't think of a better option other than deprecating the 1 arg constructor, and forcing clients to explicitly define their behaviour going forward. |
Me neither. Let's let this sit for a bit and see if anyone else has comments. |
One concern is that this diff changed the default behavior to not passing 'enable', which will break the users who are already using service discovery server and client with the enable field. If I understand correctly, there are 4 cases:
The first 2 cases we don't need to worry about, the 3rd one will fail because the server cannot recognize the new enable field, the 4th will fail because the client cannot recognize the new field, sounds like we only need to change the code to be able to ignore unknown field, what's the reason of having OldServiceInstance? |
@lvfangmin the problem as reported is existing clients in a distributed system. That code cannot be updated and the previous versions of Curator's ServiceDiscovery ObjectMapper wasn't set to ignore unknown fields. The user was doing a rolling upgrade or just wanted to have clients with different versions deployed, etc. |
I see, do we have the client version in the request, if we have it then maybe we can make the server code backward compatible based on the client version. |
But what about case when service uses old version (or compat mode) and client has new code? After de-serialisation |
@betalb we cannot fully control the version of Curator being used on client side during gradually roll out, but I suppose we we can control the version of Curator service running on the server side, correct me if I'm wrong. |
IIRC, the deserialization code assumes that if enabled is missing or null, it's treated as "true". So I think there's no issue here. |
I think not, since there isn't even really a "request" from the client to the server. The client is just deserializing json written by the server to a znode. |
This patch looks good to me. NB: I have only read it, not tested it.
I think change is good, but I would do it in the next 'major' release like 3.0.0, since it's a breaking change for older clients. |
Oh wait, there's already a Curator 3.0. Well, whatever the next 'major' release would be in the Curator numbering scheme. Maybe 2.13.0 or 4.0 depending on what the Curator community would expect. In my case I'd prefer 4.0 since I like to think that I can upgrade 'minor' versions without expecting anything to break. |
@gianm Can you point out where this check is done. I've looked into |
@betalb, in https://github.com/apache/curator/blob/e7f55f89056f1447cb2ed73a0cdfd66759e11f91/curator-x-discovery/src/main/java/org/apache/curator/x/discovery/ServiceInstance.java, the default constructor sets enabled to true: /**
* Inits to default values. Only exists for deserialization
*/
ServiceInstance()
{
this("", "", null, null, null, null, 0, ServiceType.DYNAMIC, null, true);
} And if "enabled" is not present in the JSON object then it should stay that way. I'm not sure if there's a unit test for this behavior though, I didn't check that. If not then it would be good to have one just to be sure. @Randgalt do you know if there is one? |
You mean a test that proves that "enable" isn't in the JSON? Not explicitly but it's implied by the old serializer tests. |
I meant a test that proves that deserializing some JSON without "enable" into |
OK - I added a test for forward compatibility. |
@Randgalt what's your feeling about how this patch makes it to a release? For Druid were waiting for this in order to upgrade Curator. |
Folks who are happy with the patch as it is please put a 👍 on this message. If anyone is against this patch, please reply. |
I think that what we've got is probably the best compromise we're going to get. |
So, any last comments before this is merged? |
Was this supposed to be backported to the 2.x series? We upgraded some clients to 2.12.0 and ran into this. |
It was marked to go into 2.13.0 but was never released as we moved to a common binary for ZK 3.4 and 3.5. Have you tried 4.0.0? see: http://curator.apache.org/zk-compatibility.html |
|
Here is a potential fix for the issue folks. Let me know what you think...
For users who don't want the new "enabled" field serialized, you can create a JsonInstanceSerializer in compatibility mode:
Internally, the JsonInstanceSerializer will not serialize the enabled field. There are tests to ensure that this will deserialize in old clients.
Going forward, JsonInstanceSerializer now sets ignore fields in the ObjectMapper.