michael-o commented on pull request #77: URL: https://github.com/apache/maven-resolver/pull/77#issuecomment-813444925
> > > > A left a few comments, please go through. > > > > * I have a few more homework to do because I do not fully understand all involved layers with `AdaptedReadWriteLockNamedLock` and `NamedLockFactorySupport`. I am bit worried that (Redisson) lock objects will be cached in memory although it is absolutely not always necessary because the Redission client will retrieve them anyway from the Redis instance when a lock operation is performed. > > * What is gone is the map with key lock and value integer. It has been replaced with a deque. From a technical point of view it is fine because it will retain reverse order lock release to avoid deadlocks. Though, I intentionally have created the map because I wanted to print the reentrant lock count for debugging/analysis purposes. > > * After I will have established better understanding I will run tests. > > * What I do not understand is why bind name mappers and factories, but still assemble them manually instead of having a map injects with a provider/map with name key and object value?! Isn't this duplicate work/brittle? > > * Yes, they will be cached, for as long as the lock is in use. As we implement reference counting, they will be removed once the last lock using thread releases it. Looks sound! > * we can add some debugging/analysis helpers around, if needed, but IMHO we did not lost too much here. What specifics you have on mind? What helps me a lot is that not only a lock is acquired, but how it is has been reacquired (if any) and that it has been released correctly. During my tests with Redisson I have noticed that debugging solely relying on logs is hard and it has to be good. Morever, as you know, debugging concurrent issues is almost not possible. So I prefer more log statements than less. Let me have a look at my current Redisson impl and get back to you. > * OK +1 > * 😄 ServiceLocator: as long it is present, we need these "loops and hoops", make objects work with default ctor, "constant-looking params" and other trickeries... once ServiceLocator is gone, and only DI (SISU more specifically) is used, these things will get explicit and much more cleaner and use ctor injection purely. Makes sense. Will now go over to understand your code better. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
