On Wed, Jun 3, 2015 at 1:16 PM, Kevin Burton <bur...@spinn3r.com> wrote:
> Thanks for creating the issues! > No problem. > > > The problem with my patch set, is that I’m still > > > stuck on 5.10.2. There’s a bug introduced sometime around 5.11 that > only > > > impacts the memory store. I haven’t been able to track it down yet so > I > > > can’t retarget my patches to head. > > > > > > Can you provide more details? We're using the memory store in 5.10.0, so > > we'd like to know what might bite us if we upgrade. > > > > > Advisories break when using the memory store. A warning that a null pointer > exception was caught goes to the log but the advisories aren’t raised. > > I tried to git bisect it but wasn’t able to duplicate it easily. I think > because of some git idiosyncrasy prevented me from finding the commit that > broke. > OK, thanks for sharing. Have you created a bug report for it? If not, can you do that so it doesn't get lost? > > It wouldn't; concurrent collections only protect you from corrupting the > > collection itself due to concurrent modifications; they don't protect you > > from synchronization issues between multiple collections like you'd have > > here (since you'll want to always have an element in neither collection > or > > in both, but without explicit locking you could have it in one but not > the > > other) and they don't protect you from synchronization issues where you > > observe the current state of the collection and act upon it (where it > might > > have changed between your observation and your action). > > > > > I had thought about that. I assumed that the multi map was implemented as > two concurrent maps. If each operation is concurrent and atomic then I’m > fine with that solution. > A Guava Multimap is essentially a Map<Key, List<Value>>, but without having to manage the Lists yourself. It lets you map each key to an unlimited number of values, while preserving the simple semantics of the fundamental Map operations. It's not a pair of maps (which is what it sounds like you were expecting; if I misunderstood, I apologize) nor is it essentially a Map<Key, Pair<ValueType1, ValueType2>>; it holds one homogenous set of values that just happen to have overlapping keys, not multiple sets of values where each key pairs with one value per set. You could probably implement something that held both value types for a given key with a Multimap, but you'd have to use Object as your value class to accommodate the multiple value types and you'd have to test each one's type to figure out what role it's supposed to be used in, so I don't think you'd want to. And although for a concurrent Multimap it's presumably possible to atomically remove all values for a key (though I've not needed to yet so I haven't looked), I don't believe it's possible to add multiple values for a key atomically, so I'm not sure it's possible even if you were willing to hold your nose while you did it. > I think all we need is that the method() is atomic, not the underlying > data. Maybe I’m wrong though. Perhaps there’s a race where we can try to > remove something while it’s being added? > That's what I'm not sure about; I don't know whether there are situations where we can concurrently attempt to add and remove consumers (maybe in the scenario where a broker-to-broker network connection fails over?), nor whether there are situations where we might try to get values from both maps but where only one of the two is populated because we're in the middle of an addition or a deletion. Maybe someone else who's got more experience than me with that code can say whether it's a concern... > > I've generally found that concurrent collections hurt more than they > help, > > mainly because developers assume they're magically safe in multithreaded > > applications when they're not. Guess maybe I should just switch to Scala > > so I don't have to worry about any of that... > > > > > Perhaps.. but a lot of these things are called out in the documentation. > > Kevin > If only every developer actually read the documentation... Tim
