/graylog2-server

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

Adding basic OR-matching for Streams. #1292

Merged
merged 10 commits into from Jul 14, 2015

Conversation

Reviewers
No reviews
Assignees

@bernd bernd

Labels
Projects
None yet
Milestone
No milestone
3 participants
@dennisoelkers @bernd @joschi
@dennisoelkers
Member

dennisoelkers commented Jul 8, 2015

  • Adds attribute to streams that defines their matching type
  • Matching type is AND per default if matching type is not specified
  • Allowing to specify matching type during stream creation via REST api
  • Adding basic integration tests

@dennisoelkers dennisoelkers force-pushed the feature-stream-matcher-or-operator branch 2 times, most recently from 4a99ac1 to 3494e53 Jul 8, 2015

@dennisoelkers dennisoelkers added the ready-for-review label Jul 9, 2015

dennisoelkers added some commits Jul 8, 2015

@dennisoelkers
Adding basic OR-matching for Streams. 
- Adds attribute to streams that defines their matching type
- Matching type is AND per default if matching type is not specified
- Allowing to specify matching type during stream creation via REST api
- Adding basic integration tests
a7bac4a
@dennisoelkers
Adding helper method for matching type which returns DEFAULT if null. 
Unbreaks creating a stream with an unspecified matching type.
87809db
@dennisoelkers
Storing string for matching type, adding unit tests.
c0fbfe7
@dennisoelkers
Adding license header.
ec82b2f
@dennisoelkers
Using full enum type instead of String in Stream creation request.
7441478
@dennisoelkers
Using String for request parameter/correct builder method for creation.
a37619e
@dennisoelkers
Adding more test cases.
c8be755
@dennisoelkers
Fixing error in test case.
0f69622
@dennisoelkers
Fixing NPE in test case.
Loading status checks…
db76998

@dennisoelkers dennisoelkers force-pushed the feature-stream-matcher-or-operator branch from 72d3518 to db76998 Jul 13, 2015

@bernd bernd self-assigned this Jul 13, 2015

graylog2-server/src/main/java/org/graylog2/streams/StreamRouterEngine.java
@@ -258,8 +260,11 @@ public void increment() {
}
public boolean isMatched() {
// If a stream has multiple stream rules, all of the rules have to match.
return ruleCount == matches;
switch (stream.getMatchingType()) {

This comment has been minimized.

Sign in to view
Copy link
@bernd

bernd Jul 13, 2015

Member

How about storing the matching type in a field in the constructor? That would avoid calling stream.getMatchingType() over and over again.

This comment has been minimized.

Sign in to view
Copy link
@dennisoelkers

dennisoelkers Jul 13, 2015

Member

It's a simple getter, so it's probably inlined anyway.

This comment has been minimized.

Sign in to view
Copy link
@bernd

bernd Jul 14, 2015

Member

Ah, okay. The getter does a hash map lookup, does that get inlined as well?

This comment has been minimized.

Sign in to view
Copy link
@bernd

bernd Jul 14, 2015

Member

Looks like it's getting inlined. Sorry for the noise, just want to understand it. 😃

                            @ 197   org.graylog2.streams.StreamRouterEngine$StreamMatch::isMatched (91 bytes)   inline (hot)
                              @ 7   org.graylog2.streams.StreamImpl::getMatchingType (28 bytes)   inline (hot)
                               \-> TypeProfile (2700/2700 counts) = org/graylog2/streams/StreamImpl
                                @ 6   java.util.LinkedHashMap::get (25 bytes)   inline (hot)
                                 \-> TypeProfile (2609/2609 counts) = java/util/LinkedHashMap
                                  @ 2   java.util.HashMap::getEntry (86 bytes)   inline (hot)
                                    @ 19   java.util.HashMap::hash (55 bytes)   inline (hot)
                                      @ 27   java.lang.String::hashCode (55 bytes)   inline (hot)
                                    @ 33   java.util.HashMap::indexFor (6 bytes)   inline (hot)
                                    @ 68   java.lang.String::equals (81 bytes)   (intrinsic)
                                  @ 17   java.util.LinkedHashMap$Entry::recordAccess (35 bytes)   inline (hot)
                                    @ 6   java.util.LinkedHashMap::access$000 (5 bytes)   accessor
                                    @ 23   java.util.LinkedHashMap$Entry::remove (23 bytes)   inline (hot)
                                    @ 28   java.util.LinkedHashMap::access$100 (5 bytes)   accessor
                                    @ 31   java.util.LinkedHashMap$Entry::addBefore (30 bytes)   inline (hot)
                                @ 24   org.graylog2.plugin.streams.Stream$MatchingType::valueOf (11 bytes)   inline (hot)
                                  @ 4   java.lang.Enum::valueOf (73 bytes)   inline (hot)
                                    @ 1   java.lang.Class::enumConstantDirectory (114 bytes)   too big
                              @ 12   java.lang.Enum::ordinal (5 bytes)   accessor

This comment has been minimized.

Sign in to view
Copy link
@dennisoelkers

dennisoelkers Jul 14, 2015

Member

No worries, many thanks for your thoughtful investigation! 👍

This comment has been minimized.

Sign in to view
Copy link
@joschi

joschi Jul 14, 2015

Contributor

Hm, but inlining the methods of a hash lookup is still slower than a constant/variable lookup. So if the method is called very often (as in: for every incoming message), we could still benefit from saving the matching type into a final variable in the constructor.

This comment has been minimized.

Sign in to view
Copy link
@dennisoelkers

dennisoelkers Jul 14, 2015

Member

It will change with the next PR related to the new stream router engine anyway.
graylog2-server/src/main/java/org/graylog2/streams/StreamServiceImpl.java
@@ -95,6 +95,7 @@ public Stream create(CreateStreamRequest cr, String userId) {
streamData.put(StreamImpl.FIELD_CREATOR_USER_ID, userId);
streamData.put(StreamImpl.FIELD_CREATED_AT, Tools.iso8601());
streamData.put(StreamImpl.FIELD_CONTENT_PACK, cr.contentPack());
streamData.put(StreamImpl.FIELD_MATCHING_TYPE, cr.matchingType().toString());

This comment has been minimized.

Sign in to view
Copy link
@bernd

bernd Jul 13, 2015

Member

matchingType in CreateStreamRequest is @Nullable so this could fail.

This comment has been minimized.

Sign in to view
Copy link
@dennisoelkers

dennisoelkers Jul 13, 2015

Member

Actually not, as in the creator method of the DTO the default is used then.

This comment has been minimized.

Sign in to view
Copy link
@bernd

bernd Jul 13, 2015

Member

Aye, missed that.

This comment has been minimized.

Sign in to view
Copy link
@joschi

joschi Jul 14, 2015

Contributor

In this case the @Nullable on the getter should be removed as only the method argument in the factory method is nullable but not the actual attribute: https://github.com/Graylog2/graylog2-server/pull/1292/files#diff-1480db0af34470adda8a855ab7063cb5R49

This comment has been minimized.

Sign in to view
Copy link
@dennisoelkers

dennisoelkers Jul 14, 2015

Member

Totally correct.

bernd added a commit that referenced this pull request Jul 14, 2015

@bernd
Merge pull request #1292 from Graylog2/feature-stream-matcher-or-oper… 
…ator

Adding basic OR-matching for Streams.
Loading status checks…
6a195e6

@bernd bernd merged commit 6a195e6 into master Jul 14, 2015
2 checks passed

2 checks passed

ci Jenkins build graylog2-server-integration-pr 47 has succeeded
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@bernd bernd deleted the feature-stream-matcher-or-operator branch Jul 14, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment