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.

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

Refactor PathTrie and RestController to use a single trie for all methods #25459

Merged
merged 5 commits into from Jul 5, 2017

Conversation

Projects
None yet
5 participants
@dakrone
Copy link
Member

commented Jun 28, 2017

This changes PathTrie and RestController to use a single PathTrie for all
endpoints, it also allows retrieving the endpoints' supported HTTP methods more
easily.

This is a spin-off and prerequisite of #24437

Refactor PathTrie and RestController to use a single trie for all met…
…hods

This changes `PathTrie` and `RestController` to use a single `PathTrie` for all
endpoints, it also allows retrieving the endpoints' supported HTTP methods more
easily.

This is a spin-off and prerequisite of #24437
/**
* Encapsulate multiple handlers for the same path, allowing different handlers for different HTTP verbs.
*/
public class MethodHandlers {

This comment has been minimized.

Copy link
@rjernst

rjernst Jun 29, 2017

Member

Could we instead have RestHandler have optional overridable methods for each verb? That would seem less error prone (and greatly reduce the number of classes we define from what we currently have).

This comment has been minimized.

Copy link
@s1monw

s1monw Jun 29, 2017

Contributor

I think, @dakrone correct me if I am wrong, that in #24437 we need to know if a rest handler can handle a certain verb to ensure we send the right error back. That might be the reason why we need to be explicit about it. But maybe there is a way to do this?

This comment has been minimized.

Copy link
@dakrone

dakrone Jun 29, 2017

Author Member

I think, @dakrone correct me if I am wrong, that in #24437 we need to know if a rest handler can handle a certain verb to ensure we send the right error back. That might be the reason why we need to be explicit about it. But maybe there is a way to do this?

That's correct, that's the reason for this, the only way I can think of to do it on the handler itself is collapsing every handler for the same rest endpoint into the same class, and then duplicating methods to determine which verbs are supported. I don't think it would be cleaner at all

@s1monw
Copy link
Contributor

left a comment

left some suggestions and comments

if (node == null) {
node = children.get(wildcard);
if (node == null) {
if (trieMatchingMode == TrieMatchingMode.WILDCARD_NODES_ALLOWED) {

This comment has been minimized.

Copy link
@s1monw

s1monw Jun 29, 2017

Contributor

maybe you wanna use a switch here?

This comment has been minimized.

Copy link
@dakrone

dakrone Jun 29, 2017

Author Member

I tried to change this to a switch, but it's not strictly a switch due to the additional if conditions in the branches below and the else condition, I think it's much cleaner as an if tree

// is a child wildcard node, use the child wildcard node
if (index + 1 == path.length && node.value == null && children.get(wildcard) != null) {
if (index + 1 == path.length && node.value == null && children.get(wildcard) != null
&& trieMatchingMode != TrieMatchingMode.WILDCARD_ROOT_NODES_ALLOWED

This comment has been minimized.

Copy link
@s1monw

s1monw Jun 29, 2017

Contributor

I like it when we have an explicitly named EnumSet for conditions like this. It's easier to read and understand the code?


import static java.util.Collections.emptyMap;
import static java.util.Collections.unmodifiableMap;

public class PathTrie<T> {

enum TrieMatchingMode {

This comment has been minimized.

Copy link
@s1monw

s1monw Jun 29, 2017

Contributor

I wonder, if we would always retrieve all matching one (recall optimized) and then sort once we have them by the conditions we have here would that be simpler code wise? Also down the road, not in this PR should we use an FST <String, int> that we could score by some predifined value to get the sorted order by default and safe a boat load of objects? the output of the fst would then just point into an array to get the handler...

This comment has been minimized.

Copy link
@dakrone

dakrone Jul 5, 2017

Author Member

I wonder, if we would always retrieve all matching one (recall optimized) and
then sort once we have them by the conditions we have here would that be
simpler code wise?

We currently can't easily do this due to not knowing which handler matched for
the params. For example, let's say the second TrieMatchingMode matched, but
from the perspective of the caller (RestController), it needs both the
MethodHandlers object and the parameters that were parsed out of the URI for
the RestRequest. It can't use the parameters from the first handler because
they might be a different map that was parsed out.

This is why I used an Iterator with a param supplier, so that they could be
"reset" between invocations, we essentially try to match the request to a
handler multiple times, mutating the RestRequest each time and resetting it if
we didn't find an appropriate handler.

I do think it would be worth investigating this, however, I think it should be
combined with making PathTrie use a builder pattern and FST since otherwise
we'll change this whole class around only to have to basically rewrite it for
using an FST.

If it's okay with you, I'd like to tackle this in subsequent work that is more
focused, rather than blocking the improve REST PR for internal cleanup. Is that
alright?

This comment has been minimized.

Copy link
@s1monw

s1monw Jul 5, 2017

Contributor

sure thing, I didn't imply to change this now

* </pre>
* allowing the value to be updated if desired.
*/
public void insertOrUpdate(String path, T value, BiFunction<T, T, T> updater) {

This comment has been minimized.

Copy link
@s1monw

s1monw Jun 29, 2017

Contributor

would this be a perfect example to use a bulider pattern? I mean that would make it immutable which is what we want at some point? we can keep the nodes mutability for simplicity? WDYT?

This comment has been minimized.

Copy link
@dakrone

dakrone Jul 5, 2017

Author Member

I think that would be worth trying, but it would be a large undertaking. Since this is blocking a breaking change for 6.0, would you be okay with me looking at that in a follow-up?

This comment has been minimized.

Copy link
@s1monw

s1monw Jul 5, 2017

Contributor

yeah ++

@dakrone

This comment has been minimized.

Copy link
Member Author

commented Jul 5, 2017

@s1monw I pushed a commit addressing one of your comments and left responses for the others, thanks for taking a look!

@dakrone dakrone requested a review from s1monw Jul 5, 2017

@s1monw

s1monw approved these changes Jul 5, 2017

Copy link
Contributor

left a comment

I like it! I left some minors

* @param handler The handler to actually execute
*/
public void registerHandler(RestRequest.Method singleMethod, String path, RestHandler handler) {
RestRequest.Method[] methods = new RestRequest.Method[1];

This comment has been minimized.

Copy link
@s1monw

s1monw Jul 5, 2017

Contributor

why do you construct this array rather thank just passing it on to the var args? and why do we have this extra method wouldn't it be ok to just have the one with the var args?

This comment has been minimized.

Copy link
@dakrone

dakrone Jul 5, 2017

Author Member

Whoops yeah I'll remove the extra one

// Between retrieving the correct path, we need to reset the parameters,
// otherwise parameters are parsed out of the URI that aren't actually handled.
final Map<String, String> originalParams = new HashMap<>(request.params());
return handlers.retrieveAll(getPath(request), () -> {

This comment has been minimized.

Copy link
@s1monw

s1monw Jul 5, 2017

Contributor

👍

/**
* Encapsulate multiple handlers for the same path, allowing different handlers for different HTTP verbs.
*/
public class MethodHandlers {

This comment has been minimized.

Copy link
@s1monw

s1monw Jul 5, 2017

Contributor

I havent' looked closely but can this class be package private and final?

This comment has been minimized.

Copy link
@dakrone

dakrone Jul 5, 2017

Author Member

Definitely can!

@dakrone dakrone merged commit 30b5ca7 into elastic:master Jul 5, 2017

1 of 2 checks passed

elasticsearch-ci Build finished.
Details
CLA Commit author has signed the CLA
Details

@dakrone dakrone referenced this pull request Jul 5, 2017

Open

Cleanup PathTrie #25563

@dakrone

This comment has been minimized.

Copy link
Member Author

commented Jul 5, 2017

Thanks for the review @s1monw! I opened #25563 as a followup reminder with the further PathTrie changes

jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Jul 6, 2017

Merge branch 'master' into multiple-settings-validation
* master:
  Refactor PathTrie and RestController to use a single trie for all methods (elastic#25459)
  Switch indices read-only if a node runs out of disk space (elastic#25541)

@colings86 colings86 added v6.0.0-beta1 and removed v6.0.0 labels Jul 31, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.