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

add backwards compat mode for frontCoded stringEncodingStrategy #13988

Merged
merged 3 commits into from
Mar 28, 2023

Conversation

clintropolis
Copy link
Member

@clintropolis clintropolis commented Mar 28, 2023

Description

Follow up to #13854, to add a backwards compatibility mode for users of Druid 25.0 to upgrade to Druid 26.0 in a manner that allows for errorless rolling upgrades and downgrades by writing out the older format version.

Release note

The frontCoded type of stringEncodingStrategy on indexSpec, has been improved with a new segment format version which typically has faster read speeds and reduced segment size. However, this improvement is backwards incompatible with Druid 25.0, so a new formatVersion option has been added, which defaults to the new 1, but can be specified as 0 to allow migration to Druid 26.0 by the way of writing out the older format until the ability to rollback to Druid 25.0 is no longer determined to be needed.


This PR has:

  • been self-reviewed.
  • added documentation for new or modified features or behaviors.
  • a release note entry in the PR description.
  • added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
  • added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
  • been tested in a test Druid cluster.

Comment on lines 45 to 51
public void testFrontCodedDefaultSerde() throws JsonProcessingException
{
StringEncodingStrategy frontCoded = new StringEncodingStrategy.FrontCoded(null, null);
String there = JSON_MAPPER.writeValueAsString(frontCoded);
StringEncodingStrategy andBackAgain = JSON_MAPPER.readValue(there, StringEncodingStrategy.class);
Assert.assertEquals(frontCoded, andBackAgain);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

How about validate that this does a v1 and add some comments that help indicate that when/if that test breaks, it means default behavior changed and we need to think through the deployment model.

Also, then add a test that explicitly does v1 as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

added FrontCodedIndexed.DEFAULT_VERSION (and also moved DEFAULT_BUCKET_SIZE here), and test for both of them as well as explicit test for v1

Copy link
Contributor

@imply-cheddar imply-cheddar left a comment

Choose a reason for hiding this comment

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

Just one small nit. Please, in a comment on StringEncodingStrategyTest.testFrontCodedDefaultSerde, indicate that a failure there should either also occur in one of the other methods (because they are doing the default behavior too OR it's indicative that the code changed such that the default behavior is now different than it used to be and that we should ensure that the change happening is kosher for compatibility.

@clintropolis clintropolis merged commit 2219e68 into apache:master Mar 28, 2023
@clintropolis clintropolis deleted the front-coded-backwards-compat branch March 28, 2023 21:44
@clintropolis clintropolis added this to the 26.0 milestone Apr 10, 2023
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.

2 participants