-
Notifications
You must be signed in to change notification settings - Fork 5.9k
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
rgw/notification: Store the value of persistent_queue
for existing topics and continue commiting events for all topics subscribed to given bucket
#57536
base: main
Are you sure you want to change the base?
Conversation
src/rgw/rgw_rest_pubsub.cc
Outdated
// initialize the persistent queue's location, using ':' as the namespace | ||
// delimiter because its inclusion in a TopicName would break ARNs | ||
dest.persistent_queue = | ||
string_cat_reserve(get_account_or_tenant(s->owner.id), ":", topic_name); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think this should just copy dest.persistent_queue = topic->dest.persistent_queue
. the topic we're overwriting may have been a legacy one whose queue doesn't include the tenant namespace
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would it not be better in way, the user now gets migrated to correct queue name ?
just that old queue will remain there in the system
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not unless we have a way to clean up the old queue. the notification thread would keep polling on it forever
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
may be this code will still not make use of the new queue name even if it exist
if (struct_v >= 7) {
decode(persistent_queue, bl);
} else if (persistent) {
// persistent topics created before v7 did not support tenant namespacing.
// continue to use 'arn_topic' alone as the queue's rados object name
persistent_queue = arn_topic;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think this should just copy
dest.persistent_queue = topic->dest.persistent_queue
. the topic we're overwriting may have been a legacy one whose queue doesn't include the tenant namespace
done
aa1efca
to
05bb79e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks correct to me. is there a test case we can extend with a re-create to verify that persistent topics still work?
looked at |
can we add that to one of the existing persistent notification tests? |
…persistent topic. Fixes https://tracker.ceph.com/issues/66097 Signed-off-by: kchheda3 <kchheda3@bloomberg.net>
…opics subscribed to bucket even if there is failure to load any single topic . Fixes https://tracker.ceph.com/issues/66097 Signed-off-by: kchheda3 <kchheda3@bloomberg.net>
05bb79e
to
9f4ea76
Compare
updated the existing test case to re-create the topic and verify the persistent_queue is correctly populated. |
Fixes https://tracker.ceph.com/issues/66097
Checklist
Show available Jenkins commands
jenkins retest this please
jenkins test classic perf
jenkins test crimson perf
jenkins test signed
jenkins test make check
jenkins test make check arm64
jenkins test submodules
jenkins test dashboard
jenkins test dashboard cephadm
jenkins test api
jenkins test docs
jenkins render docs
jenkins test ceph-volume all
jenkins test ceph-volume tox
jenkins test windows
jenkins test rook e2e