Posted by deepakjois:
deepakjois commented :
Looking at the code, it does not seem straightforward to use Concurrent maps. Some points to consider
- LinRead is an immutable protobuf object, so we cannot really modify the map inside. We will always need to create a new copy when we need to modify it.
- We try to follow the pattern of the Go client code as far as possible to keep things consistent. The Go code uses a mutex lock around the LinRead. Using
synchronizedmethods is the closest possible approximation in Java. We could try using locks explicitly if we find thatsynchronizedisnt performing well. - It is possible to use a ConcurrentMap but for that we need to use our own variant of the LinRead protobuf class. Using a ConcurrentMap would be justified if we see some significant perf gains over using
synchronizedor explicit locks. So for now, I would favor keeping the code simple. Maybe at some point later we could add some benchmarks and compare the approaches.
manishrjain commented :
If we need to create a copy of LinReads every time it’s modified, then it might make sense to avoid that by using a map, and generate LinReads protobuf when required.
deepakjois commented :
This would also affect the TxnContext protobuf class, which contains a LinRead. So we would effectively need to create another class to track the TxnContext class as well.
I am not against this change, but the bigger problem here is that unlike in Go, protobuf objects are immutable in Java. If we have to avoid copying them, we should do that consistently. That would involve more complexity in the code. It would also mean we depart more from the Go client.
We can consider doing this once the interfaces have stabilized. I would first focus on getting things to work correctly.
manishrjain commented :
Ok. Makes sense.
deepakjois commented :
We use a lock now, and that seems sufficient. I thought about this, and looking at how the code has evolved, using a concurrent map will make it unncessarily complicated. Resolving for now.