Skip to content
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

Fix SMB producer compatibility issues #4667

Merged
merged 5 commits into from
Jan 25, 2023
Merged

Fix SMB producer compatibility issues #4667

merged 5 commits into from
Jan 25, 2023

Conversation

clairemcginty
Copy link
Contributor

Unfortunately the concept of "version" in scio-smb was not well designed to start with so we find ourselves running into issues where consumers on older versions will break on producer updates. This is not an ideal solution but we can go forward with the mandate that all changes to the SMB metadata format must be backwards compatible... at least pending a thorough reworking of versioning semantics.

Comment on lines 209 to 210
// version 1 is backwards compatible with version 0
&& (this.version <= 1 && other.version <= 1)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be rolled back to original 0.11 code instead of being deleted ?

&& this.version == other.version

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My thought process was that if a producer has already migrated to 0.12.1 and produced a few days of data, then upgrades to 0.12.2, they’ll have a mix of version: 1 and version: 0 in their metadata. If we make the version check strict again, the consumers will upgrade to 0.12.2 and break on version: 1 data :(

maybe i can keep the check the way it was (0 and 1 are compatible) and just make a note that the next version bump should be to 2.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes we should re-introduce some checks otherwise the release will accept any version

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good - updated

@codecov
Copy link

codecov bot commented Jan 25, 2023

Codecov Report

Merging #4667 (4d65b90) into main (931d78c) will increase coverage by 0.00%.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #4667   +/-   ##
=======================================
  Coverage   60.64%   60.65%           
=======================================
  Files         286      286           
  Lines       10453    10453           
  Branches      694      694           
=======================================
+ Hits         6339     6340    +1     
+ Misses       4114     4113    -1     
Impacted Files Coverage Δ
...src/main/scala/com/spotify/scio/coders/Coder.scala 85.84% <0.00%> (+0.44%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

* bumping version: Scio versions prior to 0.12.1 perform a version compatibility check on SMB
* partitions which may fail if the SMB producer bumps to an incompatible version.
*
* <p>The next version bump should be to: 2
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the <p> tag was added by javafmt 🤷‍♀️

@clairemcginty clairemcginty merged commit dcaac2b into main Jan 25, 2023
@clairemcginty clairemcginty deleted the smb_compat branch January 25, 2023 14:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants