Skip to content

Conversation

@StefanRRichter
Copy link
Contributor

This PR adds a test to ensure the order of elements in enum StateDescriptor.Type which is important for the serialization format.

@StefanRRichter
Copy link
Contributor Author

cc @StephanEwen

@StephanEwen
Copy link
Contributor

StephanEwen commented Jan 11, 2017

Looks good, but is that not breaking it now, by changing the order?
Or is it only breaking between 1.2-RC0 and 1.2RC1?

I would almost suggest to not reorder now - it appears users have jumped onto the RC0 already and this is not really a crucial fix (more a code beautification)

@StefanRRichter
Copy link
Contributor Author

It only breaks w.r.t. to the previous dev master branch, right? I just preferred to have UNKNOWN at position 0 because the future way of adding enum entries is basically "append only" and having this special state in the middle then would look weird.

@StephanEwen
Copy link
Contributor

StephanEwen commented Jan 11, 2017

When was that enum introduced? Can we get a feeling how many people are already using this?
There are quite some users on the dev branch.

@StefanRRichter
Copy link
Contributor Author

It was introduced by middle/end of december 16. The benefit is also just for esthetic reasons. Would you like me to restore the old order?

@StephanEwen
Copy link
Contributor

If it is in so shortly, lets make it clean and keep the new order...

@StephanEwen
Copy link
Contributor

Merging this...

StephanEwen pushed a commit to StephanEwen/flink that referenced this pull request Jan 11, 2017
StephanEwen pushed a commit to StephanEwen/flink that referenced this pull request Jan 11, 2017
StephanEwen pushed a commit to StephanEwen/flink that referenced this pull request Jan 11, 2017
@asfgit asfgit closed this in aa0ef2a Jan 11, 2017
liuyuzhong7 pushed a commit to liuyuzhong7/flink that referenced this pull request Jan 17, 2017
@StefanRRichter StefanRRichter deleted the fix-descriptor-type-order branch January 18, 2017 13:53
joseprupi pushed a commit to joseprupi/flink that referenced this pull request Feb 12, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants