lhotari commented on pull request #9850:
URL: https://github.com/apache/pulsar/pull/9850#issuecomment-796498388
It's great that the thread safely of the data structures is now handled so
we won't run into infinite loops which would happen with plain HashMaps. With
the ConcurrentHashMaps, it will also be possible to handle data consistency
issues.
There are some remaining data consistency ("lost update") issues when
mutating `AuthPolicies` maps, for example here:
https://github.com/apache/pulsar/blob/e24645852edeee6aa0850ce814384bedd6518e7a/pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/NamespacesBase.java#L691-L694
https://github.com/apache/pulsar/blob/1fab5aa69446b4c2eaebb615d35fe9f5a509ba4c/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authorization/PulsarAuthorizationProvider.java#L296-L299
https://github.com/apache/pulsar/blob/782372d8f51984f18de3b102cfb3c00c6dedb2bb/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authorization/PulsarAuthorizationProvider.java#L344-L356
https://github.com/apache/pulsar/blob/76883d455b44b7b0b73684d0212002397dbb9b2c/pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java#L324-L331
https://github.com/apache/pulsar/blob/76883d455b44b7b0b73684d0212002397dbb9b2c/pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java#L391-L395
There might be some other locations besides the ones above.
Would it help to use `Map.compute` instead of `.get(..).removeAll(...)` and
`.put(..,..)` for mutations? Isn't `Map.compute` atomic for
`ConcurrentHashMap`? WDYT?
@315157973 are you thinking of addressing the mutations of the
`AuthPolicies` maps in this PR or separately?
----------------------------------------------------------------
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:
[email protected]
