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

Add list of stream IDs to Message#toElasticSearchObject() #3277

Merged
merged 4 commits into from Jan 3, 2017

Conversation

@joschi
Copy link
Contributor

@joschi joschi commented Jan 2, 2017

The Message#toElasticSearchObject() only populated the streams field of the generated Elasticsearch document if the Message.streams field was filled, but not if the streams field in the Message.fields map was being used.

This lead to inconsistent "exports" of the internal Message representation.

Jochen Schalanda
@joschi joschi added this to the 2.2.0 milestone Jan 2, 2017
@joschi joschi requested a review from bernd Jan 2, 2017
@bernd bernd self-assigned this Jan 2, 2017
@@ -216,6 +217,7 @@ public DateTime getTimestamp() {

obj.put(FIELD_MESSAGE, getMessage());
obj.put(FIELD_SOURCE, getSource());
obj.put(FIELD_STREAMS, getStreamIds());

This comment has been minimized.

@bernd

bernd Jan 2, 2017
Member

This doesn't fix the issue with the archive restore.

In the restore case the stream IDs are in the fields map but the streams set is empty and thus the stream IDs in fields will be cleared.

This comment has been minimized.

@bernd

bernd Jan 2, 2017
Member

Also, I am not sure if we really should use getStreamIds() here because it's using stream functions. I guess those create more objects than the simple for loop we had before?

Jochen Schalanda added 2 commits Jan 3, 2017
Jochen Schalanda
For performance and less object creation. Maybe.
Jochen Schalanda
}

return new ArrayList<>(streamIds);

This comment has been minimized.

@bernd

bernd Jan 3, 2017
Member

How about changing the signature of getStreamIds() to return a Set or create a different method for this case? This would avoid the additional list creation.

I just want to be super careful here because this stuff is called for every message that's written into Elasticsearch and now we are creating one HashSet and one ArrayList for every call. Previously this was one ArrayList if the message had streams.

This comment has been minimized.

@joschi

joschi Jan 3, 2017
Author Contributor

I'm totally for changing the signature of this method, even if it's breaking the API in this case.

@SuppressWarnings("unchecked")
final List<String> streamIds = getFieldAs(List.class, FIELD_STREAMS);
return new ArrayList<>(streamIds);
streamField = getFieldAs(List.class, FIELD_STREAMS);

This comment has been minimized.

@bernd

bernd Jan 3, 2017
Member

I found out why this still fails with the archive restore. The archive creates a message with a Set<String> for the streams field in the map.

This comment has been minimized.

@bernd

bernd Jan 3, 2017
Member

Thanks Map<String, Object>...

This comment has been minimized.

@joschi

joschi Jan 3, 2017
Author Contributor

👍

Jochen Schalanda
@bernd
bernd approved these changes Jan 3, 2017
@bernd
Copy link
Member

@bernd bernd commented Jan 3, 2017

LGTM 👍 Also fixes the archive issue.

@bernd bernd merged commit 2e09134 into master Jan 3, 2017
4 checks passed
4 checks passed
ci-web-linter Jenkins build graylog-pr-linter-check 1233 has succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details
@bernd bernd deleted the message-toelasticsearchobject-streams branch Jan 3, 2017
@bernd bernd removed the ready-for-review label Jan 3, 2017
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

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