divijvaidya commented on PR #12940: URL: https://github.com/apache/kafka/pull/12940#issuecomment-1337109469
Thank you for your review @ijuma. I see that you back to reviewing PRs after a bit of break :) I did not understand your comment about weaker semantics. I believe that the semantics are actually stronger in the new code as explained below. Please let me know if I am mistaken. I have also provided an example of latency benefit that this code change brings in scenario of multiple connection setup. **Earlier code** 1. Return the sensor if exists 2. Create a sensor if not exists 3. Add this sensor to `sensors` map 4. Acquire a lock (computeIfAbsent does it) over the `parent` key in `childrenSensors` and append the new sensor to it. case 1: exception occurs while adding to `childrenSensors` Note that if an exception occurs at step 4, the sensor still exists in the `sensors` map. Hence, the earlier code is **not atomic!** and there might be an inconsistency between `sensors` and `childrenSensors` where `sensors` may have more entries than `childrenSensors`. case 2: exception occurs while adding to `sensors` Note that if an exception occurs at step 3, no modifications will be done by the code. **New code** 1. Acquire a lock over the sensor name in `sensor` map 2. Return the sensor if exists 3. Create a sensor if not exists 4. Acquire a lock (computeIfAbsent does it) over the `parent` key in `childrenSensors` and append the new sensor to it. 5. Add this sensor to `sensors` map case 1: exception occurs while adding to `childrenSensors` Note that if an exception occurs at step 4, the new sensor **will not be added** to the `sensors` map. **This semantic is stronger than earlier code** since, it ensures there won't be a scenario where where `sensors` may have more entries than `childrenSensors`. case 2: exception occurs while adding to `sensors` An exception could occur when creating sensor (at `new Sensor`) in which case no value will be entered into `childrenSensors` and if `new Sensor` is successful, there can never be a NPE or a Class cast exception. Hence, there is no scenario where a sensor will be added to `childrenSensors` and not to `sensors`. **Advantages of new code** 1. Allows concurrent access (even for `get` scenarios) into `sensors` map. `sensors` map is accessed in the connection initialisation code path in `Selector.java` by multiple network threads. Improving lock contention for this path will improve connection setup time for scenarios when a burst of new connection setup is received (e.g. in case of client restart). 2. New code has stronger atomic semantics as demonstrated in case 1 of new code above. -- 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. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
