Skip to content

router: thread metadata_matches through to RouteEntryImplBase#1836

Merged
htuch merged 5 commits intoenvoyproxy:masterfrom
turbinelabs:slb/route-metadata-matches-1
Oct 11, 2017
Merged

router: thread metadata_matches through to RouteEntryImplBase#1836
htuch merged 5 commits intoenvoyproxy:masterfrom
turbinelabs:slb/route-metadata-matches-1

Conversation

@zuercher
Copy link
Copy Markdown
Member

First review split off of #1735.

This provides the interface used retrieve the RouteAction metadata_match fields, but leaves them unimlemented for now. It also provides protobuf utilities that are used to implement using values from metadata_match in STL maps.

Signed-off-by: Stephan Zuercher <stephan@turbinelabs.io>
Signed-off-by: Stephan Zuercher <stephan@turbinelabs.io>
Copy link
Copy Markdown
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

LGTM other than naming nit.

* metadata to be matched against upstream endpoints when load
* balancing, sorted lexically by name.
*/
virtual const std::vector<MetadataMatchConstSharedPtr>& metadataMatches() const PURE;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

metadataMatches() or metadataMatchers()? The former seems to suggest we've already matched something.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

How about metadataMatchCriteria()? The object doesn't really do any matching itself.

Signed-off-by: Stephan Zuercher <stephan@turbinelabs.io>
@htuch
Copy link
Copy Markdown
Member

htuch commented Oct 11, 2017

LGTM, can you fix format?

Signed-off-by: Stephan Zuercher <stephan@turbinelabs.io>
htuch
htuch previously approved these changes Oct 11, 2017
Copy link
Copy Markdown
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

looks good, small Q and nit

virtual const HashedValue& value() const PURE;
};

typedef std::shared_ptr<const MetadataMatchCriterion> MetadataMatchCriterionConstSharedPtr;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Does this need to be shared_ptr ?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

If a Route's RouteAction has metadata_match criteria and has multiple WeightedClusters with their own metadata_match criteria, the RouteAction's MetadataMatchCriterion ends being referenced by multiple MetadataMatchCriteria implementations.

That said, they could be copied instead of using a shared_ptr.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

That's fine, just wanted to check.

}

default:
RELEASE_ASSERT(false);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: NOT_IMPLEMENTED

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

or NOT_REACHED

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Oops yes that is better. Forgot about that one.

Signed-off-by: Stephan Zuercher <stephan@turbinelabs.io>
@htuch htuch merged commit bde4ad1 into envoyproxy:master Oct 11, 2017
@zuercher zuercher deleted the slb/route-metadata-matches-1 branch October 25, 2017 22:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants