Skip to content

Conversation

@guyboertje
Copy link
Contributor

also has tests

Follows on from this closed PR on core - elastic/logstash#4104

NOTES TO REVIEWERS:

  • This pseudo codec is to be used along with the multiline codec in the file input initially.
  • When a stream identity/codec is evicted, it is also flushed using the block given to the decode method which closes over the input-filter queue.
  • There will always be one or more buffered lines trapped in the multiline codec because we don't flush on close on any buffered codecs. But if the stream/codec is evicted then the trapped lines will be released.

If you have read or commented on the first PR, these are the important differences:

  • There is only one ThreadSafe::Hash that hold both the codec and its eviction time.
  • EVICT_TIMEOUT is now 1 hour, read this as - if the stream/codec was last seen more than an hour ago it can be evicted from the map. This needs to be long enough to allow all the multilines to be read and short enough to ensure flushing.
  • MAX_IDENTITIES is now 20000
  • Cleaner thread only runs when the are stream/codecs mapped

Fixes #10

Copy link

Choose a reason for hiding this comment

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

Nice use of the hash key constructor thing! I knew this thing would come in handy.

Copy link
Contributor

Choose a reason for hiding this comment

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

TIL

@andrewvc
Copy link

andrewvc commented Nov 6, 2015

So, to clarify, the intended usage is to be able to call encode(event, identity) as an extension to the current Codec API?

This currently looks great to me code-wise btw!

@andrewvc
Copy link

andrewvc commented Nov 6, 2015

@guyboertje I'd like to hop on a zoom with you monday just to make sure I have all the details straight, but it LGTM!

Copy link
Contributor

Choose a reason for hiding this comment

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

Can be removed?

@ph
Copy link
Contributor

ph commented Nov 10, 2015

Minor comments, code LGTM!

I have tested this with concurrent-ruby 0.9.1 and tests passes

@ph
Copy link
Contributor

ph commented Nov 10, 2015

@guyboertje thanks for answering my questions, for concurrent-ruby just make an issue for the migration path. I expect both gem thread_safe and concurrent-ruby to concurrently work ;)

@ph
Copy link
Contributor

ph commented Nov 10, 2015

Lgtm sorry my last comment wasn't clear enough

add identity_map_codec and tests

tidy up comments and typos

remove commented out require "concurrent"

update to v2.0.3 and amend changelog

closes 10
@elasticsearch-bot
Copy link

Merged sucessfully into master!

@ppf2 ppf2 mentioned this pull request Nov 18, 2015
@guyboertje guyboertje deleted the fix/10 branch November 19, 2015 17:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Stream identity support to properly handle multiple sources

5 participants