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

Prepared Cadence chart release 0.22.0 with server 0.22.3 #1299

Merged
merged 6 commits into from
Nov 10, 2021

Conversation

pregnor
Copy link
Member

@pregnor pregnor commented Oct 18, 2021

Q A
Bug fix? no
New feature? yes
API breaks? no
Deprecations? no
Related tickets -
License Apache 2.0

What's in this PR?

Prepared Cadence chart 0.22.0 with server 0.22.3.

  1. Updated Cadence web 3.28.4->3.28.7
  2. Removed obsolete comment about persistence maxQPS, configuration was dropped from Cadence server in the past.
  3. Renamed internal references of Cassandra to NoSQL.
  4. Added Cassandra protoversion configuration and CASSANDRA_PROTOCOL_VERSION environment variable.
  5. Updated Cadence server 0.21.3->0.22.3.
  6. Bumped Cadence Helm chart version 0.21.2->0.22.0

Why?

To fully support the latest Cadence minor version 0.22 (0.22.0, 0.22.1, 0.22.2, 0.22.3).

  1. To use the latest available Cadence web version.
  2. To clean up the chart.
  3. To keep up to date with the upstream configuration practices: Add skeleton of other nosql plugin and add dynamodb package uber/cadence#4220.
  4. To support Cassandra protocol version as environment variable and chart argument: Adding protoversion in config file uber/cadence#4263.
  5. To support the latest available Cadence server version.
  6. To be able to release a new Cadence Helm chart tag.

Additional context

I also checked whether the is_cron field or the feature flags would require any chart modification, but as much as I could tell those are internal-only changes to the Cadence server so nothing to change in the chart this time it seems (to be confirmed).

Checklist

  • Code meets the Developer Guide
  • User guide and development docs updated (if needed)
  • Related Helm chart(s) updated (if needed)

To Do

  • Wait for a review from the Cadence team to confirm it looks good.

@pregnor
Copy link
Member Author

pregnor commented Oct 18, 2021

@longquanzheng could you please take a look at the charts when you have the time to double check?

With emphasis on:

  • checking if I missed any configuration changes in Cadence server 0.22 which should be included in the chart,
  • checking if I added any configuration changes which should not be part of the chart,
  • checking if I incorrectly implemented any of the configuration changes in the chart.

We couldn't test the Cassandra changes, but for eyeballing they should be fine if I hadn't messed anything up.

(I unfortunately I cannot add you as an external reviewer, so this was my second idea to give you a chance for catching my mistakes in the chart.)

@longquanzheng
Copy link

Thanks @pregnor for updating it. It looks good to me. IsCron is schema change and feature flags is set in APIs.

@sagikazarmark
Copy link
Member

While we can certainly hide breaking config changes from users, I'm not sure it's a good idea in the long-term, because it will inevitably lead to confusion (you can configure nosql under cassandra?). I guess it's fine for now, but we should just rip the bandaid off.

@longquanzheng do you have any backward compatibility promise for configuration?

@longquanzheng
Copy link

@longquanzheng do you have any backward compatibility promise for configuration?

Everything is backward compatible -- including the nosql.PluginName. So even we don't do anything in the template for this PR, upgrading to 0.22 should also work.

Right now the convention is that we always keep backward compatibility. We already got a bunch of config changes that we wanted to get rid of:

  • use primaryClusterName instead of masterClusterName
  • use clusterGroupMetadata instead of clusterMetadata (inspred by a convo with @pregnor )
  • use clusterGroup instead of clusterInfomation
  • use clusterRedirectionPolicy instead of DCRedirectionPolicy and move it under clusterGroupMetadata
    etc.

All those are in compatibility mode today.

My thought is that in a major release, Cadence will make it required.

cc @meiliang86 @demirkayaender @Shaddoll @vytautas-karpavicius @yycptt @emrahs
@Groxx @yux0

LMK if you have a different idea about when we should do the breaking change on loading config.

@sagikazarmark
Copy link
Member

Oh, okay. I thought this was a breaking change. Breaking changes are okay in major versions, but that's usually harder in applications. Not even sure it makes sense for you to follow semver.

Copy link
Member

@sagikazarmark sagikazarmark left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @pregnor

@mikejoh
Copy link

mikejoh commented Nov 10, 2021

Great work on this @pregnor, would it be possible to merge this PR any time soon? Looks like that there's a pending reviewer left and some conflicts between branches.

Thanks!

@pregnor
Copy link
Member Author

pregnor commented Nov 10, 2021

Hi @mikejoh ,

Yes we were planning to do the merge and chart version release today.

@pregnor pregnor merged commit 5a6e2e2 into master Nov 10, 2021
@pregnor pregnor deleted the feat/cadence-0-22-3 branch November 10, 2021 10:42
@pregnor
Copy link
Member Author

pregnor commented Nov 10, 2021

Hi @mikejoh,

We merged the PR and released the new 0.22.0 chart version (tag: cadence/0.22.0), it is available from the banzaicloud-stable Helm repository (don't forget to update the repo index).

@mikejoh
Copy link

mikejoh commented Nov 11, 2021

Hi @mikejoh,

We merged the PR and released the new 0.22.0 chart version (tag: cadence/0.22.0), it is available from the banzaicloud-stable Helm repository (don't forget to update the repo index).

@pregnor Awesome! Thanks for the info and the quick responses!

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.

5 participants