Skip to content

allow user override subproperties for update properties#14217

Closed
leizhiyuan wants to merge 6 commits intoapache:masterfrom
leizhiyuan:feat/allow_override_subProperties
Closed

allow user override subproperties for update properties#14217
leizhiyuan wants to merge 6 commits intoapache:masterfrom
leizhiyuan:feat/allow_override_subProperties

Conversation

@leizhiyuan
Copy link
Contributor

@leizhiyuan leizhiyuan commented Feb 10, 2022

now entry filter uses the first consumer properties to PersistentSubscription , but when users modified their code, and change the properties, it will not take effect.

Motivation

users use entry filter to implement tag filter,

time 1 : they use tag1
time 2 : users upgrade their code, and use tag2,

We will not take effect, only restart all brokers or users restart their apps. it is impossible.

Explain here the context, and why you're making that change. What is the problem you're trying to solve.

Modifications

override the properties every time. then when users upgrade their app, this will take effect.

Verifying this change

add test case

Does this pull request potentially affect one of the following parts:

If yes was chosen, please highlight the changes

  • Dependencies (does it add or upgrade a dependency): no
  • The public API: no
  • The schema: no
  • The default values of configurations: no
  • The wire protocol: no
  • The rest endpoints: no
  • The admin cli options: no
  • Anything that affects deployment: no

Documentation

Check the box below or label this PR directly (if you have committer privilege).

Need to update docs?

  • doc-required

    (If you need help on updating docs, create a doc issue)

  • no-need-doc

    (Please explain why)

  • doc

    (If this PR contains doc changes)

@leizhiyuan leizhiyuan force-pushed the feat/allow_override_subProperties branch from dd3d2a2 to 32ee9fe Compare February 10, 2022 10:43
@github-actions
Copy link

@leizhiyuan:Thanks for your contribution. For this PR, do we need to update docs?
(The PR template contains info about doc, which helps others know more about the changes. Can you provide doc-related info in this and future PR descriptions? Thanks)

@github-actions
Copy link

@leizhiyuan:Thanks for providing doc info!

@github-actions github-actions bot added doc-not-needed Your PR changes do not impact docs and removed doc-label-missing labels Feb 10, 2022
Copy link
Contributor

@Jason918 Jason918 left a comment

Choose a reason for hiding this comment

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

How about we pass null if user don't set this. And for server, null means no change, keep the previous value?

assertTrue(properties4.containsKey("4"));
assertEquals(properties4.get("3"), "3");
assertEquals(properties4.get("4"), "4");
assertTrue(properties4.isEmpty());
Copy link
Contributor

Choose a reason for hiding this comment

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

//consumer subscribe without subscriptionProperties set, it will get the old one
This PR changed this feature. It's worth discussing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok,I will change it

Copy link
Contributor

@codelipenghui codelipenghui left a comment

Choose a reason for hiding this comment

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

The properties of the subscription should not be updated when new consumers use different subscription properties, the properties are applied to the subscription when creating the subscription if users have many consumers with different subscription properties, with this change we will apply the last consumer.

Users should change the subscription properties for an existing subscription by the admin API, not the new consumer command.

/cc @315157973

@github-actions
Copy link

The pr had no activity for 30 days, mark with Stale label.

@github-actions
Copy link

The pr had no activity for 30 days, mark with Stale label.

@tisonkun
Copy link
Member

Closed as stale, conflict, and no consensus. Please rebase and resubmit the patch if it's still relevant.

@tisonkun tisonkun closed this Dec 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

doc-not-needed Your PR changes do not impact docs lifecycle/stale Stale

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants