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

rgw/notifications: fix tag based filtering to work on all ops #38246

Merged
merged 2 commits into from Dec 17, 2020
Merged

rgw/notifications: fix tag based filtering to work on all ops #38246

merged 2 commits into from Dec 17, 2020

Conversation

yuvalif
Copy link
Contributor

@yuvalif yuvalif commented Nov 23, 2020 •
edited

fixes: https://tracker.ceph.com/issues/48321

Signed-off-by: Yuval Lifshitz ylifshit@redhat.com

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 api
  • jenkins test docs
  • jenkins render docs
  • jenkins test ceph-volume all
  • jenkins test ceph-volume tox
    ]

@yuvalif yuvalif requested a review from cbodley November 23, 2020 16:28
@yuvalif yuvalif added needs-review and removed DNM labels Nov 24, 2020
@yuvalif yuvalif changed the title [WIP] rgw/notifications: fix tag based filtering to works on all ops rgw/notifications: fix tag based filtering to works on all ops Nov 24, 2020
@yuvalif yuvalif changed the title rgw/notifications: fix tag based filtering to works on all ops rgw/notifications: fix tag based filtering to work on all ops Nov 24, 2020
@yuvalif
Copy link
Contributor Author

yuvalif commented Nov 25, 2020

multisite tests suit is teuthology (with fixed tests) is passing: http://pulpito.front.sepia.ceph.com/yuvalif-2020-11-24_13:36:00-rgw:multisite-wip-yuval-fix-48321-distro-basic-gibba/

yuvalif
yuvalif commented Dec 2, 2020
View reviewed changes
src/rgw/rgw_notify.cc Outdated
return src_obj;
}

void metadata_from_attributes(const req_state* s, rgw::sal::RGWObject* obj, KeyValueList& metadata) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

change KeyValueList to KeyValueMap

mattbenjamin
mattbenjamin approved these changes Dec 2, 2020
View reviewed changes
Copy link
Contributor

@mattbenjamin mattbenjamin left a comment

Choose a reason for hiding this comment

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

this looks great, couple of comments

src/rgw/rgw_notify.cc Outdated
return src_obj;
}

void metadata_from_attributes(const req_state* s, rgw::sal::RGWObject* obj, KeyValueList& metadata) {
Copy link
Contributor

Choose a reason for hiding this comment

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

per call, KeyValueList sounds fine ("using flat_map<...") but maybe name it something-map, so we won't think it's an assoc-list?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

src/rgw/rgw_notify.cc
// not able to decode tags
return;
}
tags = std::move(obj_tags.get_tags());
Copy link
Contributor

Choose a reason for hiding this comment

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

this is unexpected: the code assumes that KeyValueList is of the same underlying type as RGWObjTags::get_tags() returns?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure what you mean by "unexpected"?
if the type was different, then this would have been a compilation error.
BTW, if the tags types don't match there will be other places in the code where compilation would fail.

Copy link
Contributor

Choose a reason for hiding this comment

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

unexpected to me the reader, not the compiler

src/rgw/rgw_notify.cc Outdated
tags = std::move(obj_tags.get_tags());
}
}

// populate record from request
void populate_record_from_request(const req_state *s,
Copy link
Contributor

Choose a reason for hiding this comment

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

is "record" part of notification terminology? what exactly is it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the term "record" is taken from the structure here: https://docs.aws.amazon.com/AmazonS3/latest/dev/notification-content-structure.html :

{  
   "Records":[  
      {  
         "eventVersion":"2.2",
         "eventSource":"aws:s3",
         "awsRegion":"us-west-2",
...

however, we could have used the term "event" or the term "notification" (also used in the AWS doc).

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, I think match and record are a bit free of context for me, but whatevs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will change to "event" as this is what AWS uses in their doc.
"record" is just the term inside the xml

src/rgw/rgw_notify.cc
return false;
}
} else {
// try to fetch the metadata from the attributes
Copy link
Contributor

Choose a reason for hiding this comment

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

ok

src/rgw/rgw_notify.cc Outdated
// opaque data will be filled from topic configuration
}

bool match(const rgw_pubsub_topic_filter& filter, const req_state* s, const rgw::sal::RGWObject* obj, EventType event) {
bool match(const rgw_pubsub_topic_filter& filter, const req_state* s, rgw::sal::RGWObject* obj,
Copy link
Contributor

Choose a reason for hiding this comment

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

"match" is a pretty generic and context free name for what this function does?

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 is namespaced under rgw::notify.

the different "match()" functions have different prototypes according to the filter they use.
note that I used the term "match()" and not "filter()" so there is less ambiguity in whether you "filter out" or "filter in".

Copy link
Contributor

Choose a reason for hiding this comment

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

the namespace isn't very visible 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.

how about changing to notification_match() ?

then in publish_reserve() may be clearer:

if (!notification_match(topic_filter, res.s, res.object, event_type, req_tags)) {
  // notification does not apply to req_state
  continue;
}

@github-actions github-actions bot added the tests label Dec 2, 2020
@yuvalif
Copy link
Contributor Author

yuvalif commented Dec 3, 2020

jenkins retest this please

@yuvalif
rgw/notification: use the term "event" for notifications
ca1d81d 
as well as some other nameing changes for clarifications

Signed-off-by: Yuval Lifshitz <ylifshit@redhat.com>
@yuvalif
Copy link
Contributor Author

yuvalif commented Dec 9, 2020

@mattbenjamin did the name changes in the 2nd commit

@yuvalif yuvalif merged commit ae1a20a into ceph:master Dec 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattbenjamin mattbenjamin mattbenjamin approved these changes

@cbodley cbodley Awaiting requested review from cbodley

Assignees
No one assigned
Labels
bug-fix rgw tests
Projects
None yet
Milestone
No milestone
2 participants
@yuvalif @mattbenjamin