Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16169 )
Change subject: WIP: tablet: acquire/release locks in batches ...................................................................... Patch Set 2: (5 comments) Seems a bunch of tests aren't happy http://gerrit.cloudera.org:8080/#/c/16169/2/src/kudu/tablet/lock_manager.cc File src/kudu/tablet/lock_manager.cc: http://gerrit.cloudera.org:8080/#/c/16169/2/src/kudu/tablet/lock_manager.cc@256 PS2, Line 256: Process nit: maybe rename this to RemoveFromBucket() or somesuch? http://gerrit.cloudera.org:8080/#/c/16169/2/src/kudu/tablet/lock_manager.cc@283 PS2, Line 283: prefetch I'm curious how you determined that this was a point of slowness. Or is there a broader rule of thumb for when prefetching will make a difference? Also how'd you come to a batch size of 4? http://gerrit.cloudera.org:8080/#/c/16169/2/src/kudu/tablet/lock_manager.cc@383 PS2, Line 383: : void ScopedRowLock::ReleaseBatch(ArrayView<ScopedRowLock*> locks) { : if (locks.empty()) return; : for (auto* srl : locks) srl->Release(); : } I might be missing something -- is this used anywhere? http://gerrit.cloudera.org:8080/#/c/16169/2/src/kudu/tablet/lock_manager.cc@414 PS2, Line 414: void LockManager::Lock(const Slice& key, : const OpState* op, : LockManager::LockMode mode, : LockEntry** entry) { : *entry = locks_->GetLockEntry(key); : AcquireLockOnEntry(*entry, op, mode); : } : Is this only around for tests? http://gerrit.cloudera.org:8080/#/c/16169/2/src/kudu/tablet/lock_manager.cc@450 PS2, Line 450: /* : LOG(WARNING) << "Waited " << (++waited_seconds) << " seconds to obtain row lock on key " : << KUDU_REDACT(key.ToDebugString()) << " cur holder: " << cur_holder; : : TODO: reinstate the above : : */ : // TODO(unknown): would be nice to also include some info about the blocking op, : // but it's a bit tricky to do in a non-racy fashion (the other op may : // complete at any point) : } : MicrosecondsInt64 wait_us = GetMonoTimeMicros() - start_wait_us; : TRACE_COUNTER_INCREMENT("row_lock_wait_us", wait_us); : if (wait_us > 100 * 1000) { : //TRACE("Waited $0us for lock on $1", wait_us, KUDU_REDACT(key.ToDebugString())); : // TODO reinstate Were these commented out for the sake of avoiding timing calls? -- To view, visit http://gerrit.cloudera.org:8080/16169 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3cb724e953ecdf188a35181c2f91b721b3416524 Gerrit-Change-Number: 16169 Gerrit-PatchSet: 2 Gerrit-Owner: Todd Lipcon <t...@apache.org> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Andrew Wong <andrew.w...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Tue, 14 Jul 2020 01:05:17 +0000 Gerrit-HasComments: Yes
