keith-turner commented on code in PR #6025:
URL: https://github.com/apache/accumulo/pull/6025#discussion_r2624926975
##########
core/src/main/java/org/apache/accumulo/core/fate/zookeeper/DistributedReadWriteLock.java:
##########
@@ -154,22 +155,27 @@ public void lockInterruptibly() throws
InterruptedException {
@Override
public boolean tryLock() {
- if (entry == -1) {
- entry = qlock.addEntry(new ParsedLock(this.lockType(),
this.userData).getLockData());
- log.info("Added lock entry {} userData {} lockType {}", entry,
- new String(this.userData, UTF_8), lockType());
- }
- SortedMap<Long,byte[]> entries = qlock.getEarlierEntries(entry);
+ var entryVal = entry.updateAndGet(val -> {
+ if (val == -1) {
+ var newVal = qlock.addEntry(new ParsedLock(this.lockType(),
this.userData).getLockData());
Review Comment:
Not sure any changes are needed for this class. But if they are it may be
easier to make the methods of this class synchronized. The methods on atomic
long are not atomic w.r.t to execution they are only atomic for data, they do
not prevent multiple threads from executing the code block they only ensure one
thread succeeds in making a data update. So when the lambda has side effects
like `qlock.addEntry` then you have to reason about is it ok for a thread that
did not succeed to call that code. Also have to reason about the same threads
calling the lambda multiple times for the case where it does not succeed, which
can also be tricky when there are side effects. Not sure if it is ok or not in
the case, did not look in detail into what `qlock.addEntry` does.
Its usually best to avoid passing lambda that have side effects to these
methods.
##########
core/src/main/java/org/apache/accumulo/core/fate/zookeeper/DistributedReadWriteLock.java:
##########
@@ -111,7 +112,7 @@ static class ReadLock implements Lock {
QueueLock qlock;
byte[] userData;
- long entry = -1;
+ AtomicLong entry;
Review Comment:
Changes may not be needed to this class. I think it is only used by a single
thread, but not 100% sure. Did some sampling and found this usually created as
a local var in a function to try to obtain a lock in zookeeper, but the local
var is not shared among threads.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
