Add concurrent-ruby dependency. #835
Conversation
theturtle32
force-pushed
the
add-concurrent-ruby-dependency
branch
from
169f339 to
70d688e
Compare
|
@dasch - It would have been good to get this included in the v1.1.0 release... maybe as a follow-up as v1.1.1? |
|
Hmm, I'd actually rather see if we can get rid of that dependency entirely. One of the principles of ruby-kafka is that it doesn't bring in complex dependencies. |
|
I'm not sure what the rationale was for bringing in Concurrent::Map. Maybe it could be replaced with a Hash with mutex-guarded access? Is it necessary at all? |
|
I'm not exactly sure why it is needed – I missed it in my review. If you could do some digging it would be great! |
|
@lmduc is concurrent needed? |
| @@ -4,6 +4,7 @@ | |||
| require "kafka/offset_manager" | |||
| require "kafka/fetcher" | |||
| require "kafka/pause" | |||
| require "concurrent" | |||
There was a problem hiding this comment.
maybe it can be require "concurrent/map" to avoid loading the whole library, only the Map that is the needed one, wdyt?
There was a problem hiding this comment.
I’d like to get rid of the gem dependency altogether. ruby-kafka should be mostly self-contained.
There was a problem hiding this comment.
if it isn't needed, removal is a way better solution 😉
|
Looking at the code, I’m not sure why it needs to be thread safe – it doesn’t look like e.g. the fetcher thread accesses the data structure directly? |
It seems it doesn't need to be threadsafe. Via zendesk#835 (comment)
From the comment: `It seems it doesn't need to be threadsafe.` Via zendesk#835 (comment) I took a quick look and it seems it's safe to use a simple Hash Fixes: zendesk#842 Fixes: zendesk#840
|
This was merged #845 |
Somewhere between v1.0 and v1.1.0-beta1, a change was committed that uses
Concurrent::Mapbut without addingconcurrent-rubyto the dependencies in the gemspec.The tests pass because
activesupportis a development dependency, and they've hadconcurrent-rubyas one of their dependencies since version 5.0.0.But without this in the gemspec, any project that doesn't load in activesupport or concurrent-ruby itself won't work with ruby-kafka.