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.

Sign up for GitHub

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

Jump to bottom

PIP-105 add support for updating the Subscription properties #15751

Merged
merged 18 commits into from
Jun 3, 2022
Merged

PIP-105 add support for updating the Subscription properties #15751

merged 18 commits into from
Jun 3, 2022

Conversation

eolivelli
Copy link
Contributor

@eolivelli eolivelli commented May 24, 2022 •
edited
Loading

Motivation:

  • We need a way to update Subscription properties (introduced with PIP-105)

Modifications:

  • add new REST API to update Subscription Properties
  • add new PulsarAdmin API: updateSubscriptionProperties
  • add new pulsar-admin command "update-subscription-properties"

Docs:

Documentation for pulsar-admin and for the REST API are automatically generated

@eolivelli eolivelli added the doc-not-needed Your PR changes do not impact docs label May 24, 2022
@eolivelli eolivelli self-assigned this May 24, 2022
@eolivelli eolivelli force-pushed the impl/pip-105-update-subscription-properties branch from 6b2a488 to 90c3dd2 Compare May 24, 2022 12:49
@eolivelli eolivelli mentioned this pull request May 24, 2022
Merged
@eolivelli eolivelli force-pushed the impl/pip-105-update-subscription-properties branch from 24dbf27 to 77cdfcb Compare May 25, 2022 15:18
@eolivelli eolivelli added this to the 2.11.0 milestone May 26, 2022
@eolivelli
Copy link
Contributor Author

/pulsarbot rerun-failure-checks

@eolivelli
Copy link
Contributor Author

/pulsar-bot rerun-failure-checks

@eolivelli eolivelli marked this pull request as ready for review June 3, 2022 07:09
@eolivelli eolivelli changed the title PIP-105 add support for updating the Subscription properties (DRAFT) PIP-105 add support for updating the Subscription properties Jun 3, 2022
lhotari
lhotari requested changes Jun 3, 2022
View reviewed changes
Copy link
Member

@lhotari lhotari left a comment

Choose a reason for hiding this comment

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

I added a comment about making the REST API more RESTful.
I would also like to suggest implementing the GET for retrieving current properties.

pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/v2/PersistentTopics.java Outdated Show resolved Hide resolved
lhotari
lhotari requested changes Jun 3, 2022
View reviewed changes
Copy link
Member

@lhotari lhotari left a comment

Choose a reason for hiding this comment

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

Some minor log message changes. One of them mentions deleting subscription.

pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java Outdated Show resolved Hide resolved
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java Outdated Show resolved Hide resolved
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java Outdated Show resolved Hide resolved
@eolivelli @lhotari
Apply suggestions from code review
73fa1de 
Co-authored-by: Lari Hotari <lhotari@users.noreply.github.com>
lhotari
lhotari approved these changes Jun 3, 2022
View reviewed changes
Copy link
Member

@lhotari lhotari left a comment

Choose a reason for hiding this comment

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

LGTM

@lhotari lhotari requested a review from hangc0276 June 3, 2022 08:04
codelipenghui
codelipenghui approved these changes Jun 3, 2022
View reviewed changes
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.

LGTM

pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java
Comment on lines +2389 to +2392
if (!isRedirectException(ex)) {
log.error("[{}] Failed to update subscription {} from topic {}",
clientAppId(), subName, topicName, ex);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

We will not get a RedirectException here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will double check, this code is basically copy/pasted from resetCursor IIRC

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it should come from here

pulsar/pulsar-broker/src/main/java/org/apache/pulsar/broker/web/PulsarWebResource.java

Line 732 in eb1daae

throw new WebApplicationException(Response.temporaryRedirect(redirect).build());

in validateTopicOwnershipAsync

@lhotari lhotari merged commit 8e77e88 into apache:master Jun 3, 2022
@eolivelli eolivelli deleted the impl/pip-105-update-subscription-properties branch June 3, 2022 12:03
nicoloboschi pushed a commit that referenced this pull request Jun 3, 2022
@eolivelli @nicoloboschi
PIP-105 add support for updating the Subscription properties (#15751)
9dd4057 
* PIP-105 add support for updating the Subscription properties

* Implement command update-subscription-properties

* Add tests

* Add volatile

* Fix PersistentTopicTest

* PIP-105: Store Subscription properties

* Fix FilterEntryTest

* Add volatile

* Fix PersistentTopicTest

* fix ServerCnxTest test

* Switch from POST to PUT

* rename to /properties

* Apply suggestions from code review

Co-authored-by: Lari Hotari <lhotari@users.noreply.github.com>

Co-authored-by: Lari Hotari <lhotari@users.noreply.github.com>
(cherry picked from commit 8e77e88)
nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request Jun 3, 2022
@eolivelli @nicoloboschi
PIP-105 add support for updating the Subscription properties (apache#…
9bc493d 
…15751)

* PIP-105 add support for updating the Subscription properties

* Implement command update-subscription-properties

* Add tests

* Add volatile

* Fix PersistentTopicTest

* PIP-105: Store Subscription properties

* Fix FilterEntryTest

* Add volatile

* Fix PersistentTopicTest

* fix ServerCnxTest test

* Switch from POST to PUT

* rename to /properties

* Apply suggestions from code review

Co-authored-by: Lari Hotari <lhotari@users.noreply.github.com>

Co-authored-by: Lari Hotari <lhotari@users.noreply.github.com>
(cherry picked from commit 8e77e88)
(cherry picked from commit 9dd4057)
nicoloboschi added a commit that referenced this pull request Jun 8, 2022
@nicoloboschi
Fix cherry-pick issue of #15751: fix PulsarAdminToolTest#topics test
23b41cb
nicoloboschi added a commit to datastax/pulsar that referenced this pull request Jun 8, 2022
@nicoloboschi
Fix cherry-pick issue of apache#15751: fix PulsarAdminToolTest#topics…
a46ff57 
… test

(cherry picked from commit 23b41cb)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@codelipenghui codelipenghui codelipenghui approved these changes

@lhotari lhotari lhotari approved these changes

@315157973 315157973 Awaiting requested review from 315157973

@nicoloboschi nicoloboschi Awaiting requested review from nicoloboschi

@michaeljmarshall michaeljmarshall Awaiting requested review from michaeljmarshall

@hangc0276 hangc0276 Awaiting requested review from hangc0276

Assignees

@eolivelli eolivelli

Labels
cherry-picked/branch-2.10 doc-not-needed Your PR changes do not impact docs release/2.10.1
Projects
None yet
Milestone
2.11.0
Development

Successfully merging this pull request may close these issues.
4 participants
@eolivelli @lhotari @codelipenghui @nicoloboschi