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

Improved Stream Router Engine #1305

Merged
merged 19 commits into from Jul 17, 2015
Merged

Conversation

@dennisoelkers
Copy link
Member

@dennisoelkers dennisoelkers commented Jul 16, 2015

This pull request adds the new stream router engine optimized for best guess minimum cost evaluation.

@joschi joschi self-assigned this Jul 16, 2015
@joschi joschi added this to the 1.2.0 milestone Jul 16, 2015
protected final StreamFaultManager streamFaultManager;
protected final StreamMetrics streamMetrics;
protected final TimeLimiter timeLimiter;
protected final long streamProcessingTimeout;

This comment has been minimized.

@joschi

joschi Jul 16, 2015
Contributor

Why did you change the visibility of those final fields? They're only being accessed in this very class.

final List<Rule> regexRules = Lists.newArrayList();

for (Stream stream : streams) {
final Boolean sufficient = stream.getMatchingType() == Stream.MatchingType.OR;

This comment has been minimized.

@joschi

joschi Jul 16, 2015
Contributor

This could be a primitive boolean.

private final Stream stream;
private final StreamRule rule;
private final StreamRuleMatcher matcher;
private final Boolean sufficient;

This comment has been minimized.

@joschi

joschi Jul 16, 2015
Contributor

This could be a primitive boolean.

This comment has been minimized.

@joschi

joschi Jul 16, 2015
Contributor

While sufficient is shorter, I think shortCircuit or shortCircuitRule describes better, what this flag denotes in reality.

@@ -180,7 +216,7 @@ public String getFingerprint() {

for (final StreamRule streamRule : stream.getStreamRules()) {
try {
final Rule rule = new Rule(stream, streamRule);
final Rule rule = new Rule(stream, streamRule, null);

This comment has been minimized.

@joschi

joschi Jul 16, 2015
Contributor

What meaning would sufficient = null have in this case? There's also not a single check for Rule#isSufficient() == null.

This comment has been minimized.

@dennisoelkers

dennisoelkers Jul 16, 2015
Author Member

It is not relevant for the reuse of this class in the scope of the test matches, as shortcuts are not taken here and all rules need to be evaluated.

This comment has been minimized.

@joschi

joschi Jul 16, 2015
Contributor

In this case, the Rule#isSufficient() method would need to be annotated with @Nullable and null-checks being introduced on every call of that method.

This comment has been minimized.

@joschi

joschi Jul 16, 2015
Contributor

TBH, I'd prefer simply using false here and making Rule#isSufficient() return a primitive boolean.

This comment has been minimized.

@bernd

bernd Jul 16, 2015
Member

TBH, I'd prefer simply using false here and making Rule#isSufficient() return a primitive boolean.

Agreed, avoids null checks as well. Or is there a specific reason why you use Boolean instead of boolean?

This comment has been minimized.

@dennisoelkers

dennisoelkers Jul 16, 2015
Author Member

The reason was because of the reuse in the test matching logic. It is misleading to pass any value there. Null checking is a pita too. I will change to a primitive and add a comment.

This comment has been minimized.

@dennisoelkers

dennisoelkers Jul 16, 2015
Author Member

Actually I will now replace it with the matching type of the stream. The wording was annoying from the beginning and a boolean flag is just not able to properly represent what it's supposed to do.

This comment has been minimized.

@bernd

bernd Jul 16, 2015
Member

👍

@@ -83,33 +73,49 @@ public StreamRouterEngine(@Assisted List<Stream> streams,
this.streamProcessingTimeout = streamFaultManager.getStreamProcessingTimeout();
this.fingerprint = new StreamListFingerprint(streams).getFingerprint();

for (final Stream stream : streams) {
for (final StreamRule streamRule : stream.getStreamRules()) {
this.rulesList = Lists.newArrayList();

This comment has been minimized.

@joschi

joschi Jul 16, 2015
Contributor

This list should be initialized with the (maximum) size presenceRules.size() + exactRules .size() + greaterRules.size() + smallerRules.size() + regexRules.size().

final List<Rule> smallerRules = Lists.newArrayList();
final List<Rule> regexRules = Lists.newArrayList();

for (Stream stream : streams) {

This comment has been minimized.

@joschi

joschi Jul 16, 2015
Contributor

Maybe instead of sorting the stream rules manually, it would make sense to introduce some kind of complexity rating for the different types of rules, and use a comparator to sort the list of rules accordingly.

This comment has been minimized.

@dennisoelkers

dennisoelkers Jul 16, 2015
Author Member

I decided against this, because the effort to sort element by element is much higher than doing it manually.

This comment has been minimized.

@joschi

joschi Jul 16, 2015
Contributor

@@ -137,33 +143,63 @@ public String getFingerprint() {
* @return the list of matching streams
*/
public List<Stream> match(Message message) {

This comment has been minimized.

@joschi

joschi Jul 16, 2015
Contributor

Finally changing the method return type to Set<Stream> would make sense now, I guess. This would also avoid having to copy the matching result at the end of this method.

This comment has been minimized.

@dennisoelkers

dennisoelkers Jul 16, 2015
Author Member

Tried that. Not possible right now, as it would involve external changes of the API.

This comment has been minimized.

@joschi

joschi Jul 16, 2015
Contributor

This comment has been minimized.

@joschi

joschi Jul 16, 2015
Contributor

This is a bit unfortunate because right now this collection is at least copied twice for each message (one time in this method, another time in Message#setStreams()).

This comment has been minimized.

@dennisoelkers

dennisoelkers Jul 16, 2015
Author Member

Indeed. We should address this. It is beyond me why a List was chosen in the first place.

}
}, streamProcessingTimeout, TimeUnit.MILLISECONDS, true);
} catch (Exception e) {
streamFaultManager.registerFailure(rule.getStream());

This comment has been minimized.

@joschi

joschi Jul 16, 2015
Contributor

Logging the exception on DEBUG or TRACE level might help in case of errors.

This comment has been minimized.

@dennisoelkers

dennisoelkers Jul 16, 2015
Author Member

Done in the StreamFaultManager.registerFailure() method.

This comment has been minimized.

@joschi

joschi Jul 16, 2015
Contributor

The exception is not being logged in StreamFaultManager#registerFailure().

This comment has been minimized.

@dennisoelkers

dennisoelkers Jul 16, 2015
Author Member

Right!

}

private class Rule {
protected class Rule {

This comment has been minimized.

@joschi

joschi Jul 16, 2015
Contributor

Why did you change the visibility level to protected?

@@ -58,7 +59,7 @@ public void setUp() throws Exception {
when(streamFaultManager.getStreamProcessingTimeout()).thenReturn(250L);
}

private StreamRouterEngine newEngine(List<Stream> streams) {
protected StreamRouterEngine newEngine(List<Stream> streams) {

This comment has been minimized.

@joschi

joschi Jul 16, 2015
Contributor

Why was the visibility changed to protected?

return Lists.newArrayList(result);
}

private Stream matchWithTimeOut(final Message message, final Rule rule) {

This comment has been minimized.

@joschi

joschi Jul 16, 2015
Contributor

Please add @Nullable.

public Boolean isSufficient() {
return sufficient;
}

public Stream match(Message message) {

This comment has been minimized.

@joschi

joschi Jul 16, 2015
Contributor

Please add @Nullable.

}
}


This comment has been minimized.

@joschi

joschi Jul 16, 2015
Contributor

Unnecessary blank line. 😉

streamFaultManager.registerFailure(stream);
}
final Set<Stream> result = Sets.newHashSet();
final Set<Stream> blackList = Sets.newHashSet();

This comment has been minimized.

@joschi

joschi Jul 16, 2015
Contributor

These sets could be initialized with the (maximum) number of items: Sets#newHashSetWithExpectedSize(streams.size())

This comment has been minimized.

@dennisoelkers

dennisoelkers Jul 16, 2015
Author Member

Didn't do this, because it's misleading. We are not expecting it to have that size, it's is the maximum size they could have.

This comment has been minimized.

@joschi

joschi Jul 16, 2015
Contributor

}
}
}

this.rulesList = new ImmutableList.Builder<Rule>()

This comment has been minimized.

@joschi

joschi Jul 16, 2015
Contributor

Unfortunately this suffers the same problem: Having to resize the underlying array multiple times if the number of rules is high (at least once if the number of rules is greater than 4).

This comment has been minimized.

@dennisoelkers

dennisoelkers Jul 16, 2015
Author Member

I prefer immutability in this case. Remember that it's done only during initialization of the stream router (which is done only when the set of stream rules changes).

This comment has been minimized.

@joschi

joschi Jul 16, 2015
Contributor

This comment has been minimized.

@joschi

joschi Jul 17, 2015
Contributor

Having looked at the surrounding code, this class is initialized every second (even if the rules haven't been changed) and rulesList is only used internally in this class, so I think initializing the list with the correct size would yield some improvements and is worth the disadvantage(?) over having a mutable collection instead of an immutable one.

joschi added a commit that referenced this pull request Jul 17, 2015
…ed-router-engine

Improved StreamRouterEngine
@joschi joschi merged commit b26ee61 into master Jul 17, 2015
2 checks passed
2 checks passed
ci Jenkins build graylog2-server-integration-pr 60 has succeeded
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@joschi joschi deleted the feature-stream-matcher-optimized-router-engine branch Jul 17, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.