lock names usage correction (#2591)

ivanmorozov333 · web-flow · commit 3a6ff805626f · 2024-03-11T14:06:10.000+03:00
diff --git a/ydb/core/tx/columnshard/data_locks/locks/abstract.h b/ydb/core/tx/columnshard/data_locks/locks/abstract.h
@@ -1,7 +1,11 @@
 #pragma once
 #include <ydb/library/accessor/accessor.h>
-#include <vector>
+
+#include <util/generic/string.h>
+
+#include <optional>
 #include <memory>
+#include <vector>
 
 namespace NKikimr::NOlap {
 class TPortionInfo;
@@ -12,29 +16,31 @@ namespace NKikimr::NOlap::NDataLocks {
 
 class ILock {
 private:
+    YDB_READONLY_DEF(TString, LockName);
     YDB_READONLY_FLAG(ReadOnly, false);
 protected:
-    virtual bool DoIsLocked(const TPortionInfo& portion) const = 0;
-    virtual bool DoIsLocked(const TGranuleMeta& granule) const = 0;
+    virtual std::optional<TString> DoIsLocked(const TPortionInfo& portion) const = 0;
+    virtual std::optional<TString> DoIsLocked(const TGranuleMeta& granule) const = 0;
     virtual bool DoIsEmpty() const = 0;
 public:
-    ILock(const bool isReadOnly = false)
-        : ReadOnlyFlag(isReadOnly)
+    ILock(const TString& lockName, const bool isReadOnly = false)
+        : LockName(lockName)
+        , ReadOnlyFlag(isReadOnly)
     {
 
     }
 
     virtual ~ILock() = default;
 
-    bool IsLocked(const TPortionInfo& portion, const bool readOnly = false) const {
+    std::optional<TString> IsLocked(const TPortionInfo& portion, const bool readOnly = false) const {
         if (IsReadOnly() && readOnly) {
-            return false;
+            return {};
         }
         return DoIsLocked(portion);
     }
-    bool IsLocked(const TGranuleMeta& g, const bool readOnly = false) const {
+    std::optional<TString> IsLocked(const TGranuleMeta& g, const bool readOnly = false) const {
         if (IsReadOnly() && readOnly) {
-            return false;
+            return {};
         }
         return DoIsLocked(g);
     }
diff --git a/ydb/core/tx/columnshard/data_locks/locks/composite.h b/ydb/core/tx/columnshard/data_locks/locks/composite.h
@@ -8,28 +8,28 @@ class TCompositeLock: public ILock {
     using TBase = ILock;
     std::vector<std::shared_ptr<ILock>> Locks;
 protected:
-    virtual bool DoIsLocked(const TPortionInfo& portion) const override {
+    virtual std::optional<TString> DoIsLocked(const TPortionInfo& portion) const override {
         for (auto&& i : Locks) {
-            if (i->IsLocked(portion)) {
-                return true;
+            if (auto lockName = i->IsLocked(portion)) {
+                return lockName;
             }
         }
-        return false;
+        return {};
     }
-    virtual bool DoIsLocked(const TGranuleMeta& granule) const override {
+    virtual std::optional<TString> DoIsLocked(const TGranuleMeta& granule) const override {
         for (auto&& i : Locks) {
-            if (i->IsLocked(granule)) {
-                return true;
+            if (auto lockName = i->IsLocked(granule)) {
+                return lockName;
             }
         }
-        return false;
+        return {};
     }
     bool DoIsEmpty() const override {
         return Locks.empty();
     }
 public:
-    TCompositeLock(const std::vector<std::shared_ptr<ILock>>& locks, const bool readOnly = false)
-        : TBase(readOnly)
+    TCompositeLock(const TString& lockName, const std::vector<std::shared_ptr<ILock>>& locks, const bool readOnly = false)
+        : TBase(lockName, readOnly)
     {
         for (auto&& l : locks) {
             if (!l || l->IsEmpty()) {
@@ -39,8 +39,8 @@ class TCompositeLock: public ILock {
         }
     }
 
-    TCompositeLock(std::initializer_list<std::shared_ptr<ILock>> locks, const bool readOnly = false)
-        : TBase(readOnly)
+    TCompositeLock(const TString& lockName, std::initializer_list<std::shared_ptr<ILock>> locks, const bool readOnly = false)
+        : TBase(lockName, readOnly)
     {
         for (auto&& l : locks) {
             if (!l || l->IsEmpty()) {
diff --git a/ydb/core/tx/columnshard/data_locks/locks/list.h b/ydb/core/tx/columnshard/data_locks/locks/list.h
@@ -11,36 +11,42 @@ class TListPortionsLock: public ILock {
     THashSet<TPortionAddress> Portions;
     THashSet<TTabletId> Granules;
 protected:
-    virtual bool DoIsLocked(const TPortionInfo& portion) const override {
-        return Portions.contains(portion.GetAddress());
+    virtual std::optional<TString> DoIsLocked(const TPortionInfo& portion) const override {
+        if (Portions.contains(portion.GetAddress())) {
+            return GetLockName();
+        }
+        return {};
     }
-    virtual bool DoIsLocked(const TGranuleMeta& granule) const override {
-        return Granules.contains((TTabletId)granule.GetPathId());
+    virtual std::optional<TString> DoIsLocked(const TGranuleMeta& granule) const override {
+        if (Granules.contains((TTabletId)granule.GetPathId())) {
+            return GetLockName();
+        }
+        return {};
     }
     bool DoIsEmpty() const override {
         return Portions.empty();
     }
 public:
-    TListPortionsLock(const std::vector<std::shared_ptr<TPortionInfo>>& portions, const bool readOnly = false)
-        : TBase(readOnly)
+    TListPortionsLock(const TString& lockName, const std::vector<std::shared_ptr<TPortionInfo>>& portions, const bool readOnly = false)
+        : TBase(lockName, readOnly)
     {
         for (auto&& p : portions) {
             Portions.emplace(p->GetAddress());
             Granules.emplace((TTabletId)p->GetPathId());
         }
     }
 
-    TListPortionsLock(const std::vector<TPortionInfo>& portions, const bool readOnly = false)
-        : TBase(readOnly) {
+    TListPortionsLock(const TString& lockName, const std::vector<TPortionInfo>& portions, const bool readOnly = false)
+        : TBase(lockName, readOnly) {
         for (auto&& p : portions) {
             Portions.emplace(p.GetAddress());
             Granules.emplace((TTabletId)p.GetPathId());
         }
     }
 
     template <class T, class TGetter>
-    TListPortionsLock(const std::vector<T>& portions, const TGetter& g, const bool readOnly = false)
-        : TBase(readOnly) {
+    TListPortionsLock(const TString& lockName, const std::vector<T>& portions, const TGetter& g, const bool readOnly = false)
+        : TBase(lockName, readOnly) {
         for (auto&& p : portions) {
             const auto address = g(p);
             Portions.emplace(address);
@@ -49,8 +55,8 @@ class TListPortionsLock: public ILock {
     }
 
     template <class T>
-    TListPortionsLock(const THashMap<TPortionAddress, T>& portions, const bool readOnly = false)
-        : TBase(readOnly) {
+    TListPortionsLock(const TString& lockName, const THashMap<TPortionAddress, T>& portions, const bool readOnly = false)
+        : TBase(lockName, readOnly) {
         for (auto&& p : portions) {
             const auto address = p.first;
             Portions.emplace(address);
diff --git a/ydb/core/tx/columnshard/data_locks/locks/snapshot.h b/ydb/core/tx/columnshard/data_locks/locks/snapshot.h
@@ -11,15 +11,21 @@ class TSnapshotLock: public ILock {
     const TSnapshot SnapshotBarrier;
     const THashSet<TTabletId> PathIds;
 protected:
-    virtual bool DoIsLocked(const TPortionInfo& portion) const override {
-        return PathIds.contains((TTabletId)portion.GetPathId()) && portion.RecordSnapshotMin() <= SnapshotBarrier;
+    virtual std::optional<TString> DoIsLocked(const TPortionInfo& portion) const override {
+        if (PathIds.contains((TTabletId)portion.GetPathId()) && portion.RecordSnapshotMin() <= SnapshotBarrier) {
+            return GetLockName();
+        }
+        return {};
     }
-    virtual bool DoIsLocked(const TGranuleMeta& granule) const override {
-        return PathIds.contains((TTabletId)granule.GetPathId());
+    virtual std::optional<TString> DoIsLocked(const TGranuleMeta& granule) const override {
+        if (PathIds.contains((TTabletId)granule.GetPathId())) {
+            return GetLockName();
+        }
+        return {};
     }
 public:
-    TSnapshotLock(const TSnapshot& snapshotBarrier, const THashSet<TTabletId>& pathIds, const bool readOnly = false)
-        : TBase(readOnly)
+    TSnapshotLock(const TString& lockName, const TSnapshot& snapshotBarrier, const THashSet<TTabletId>& pathIds, const bool readOnly = false)
+        : TBase(lockName, readOnly)
         , SnapshotBarrier(snapshotBarrier)
         , PathIds(pathIds)
     {
diff --git a/ydb/core/tx/columnshard/data_locks/manager/manager.cpp b/ydb/core/tx/columnshard/data_locks/manager/manager.cpp
@@ -3,8 +3,9 @@
 
 namespace NKikimr::NOlap::NDataLocks {
 
-void TManager::RegisterLock(const TString& processId, const std::shared_ptr<ILock>& lock) {
-    AFL_VERIFY(ProcessLocks.emplace(processId, lock).second)("process_id", processId);
+void TManager::RegisterLock(const std::shared_ptr<ILock>& lock) {
+    AFL_VERIFY(lock);
+    AFL_VERIFY(ProcessLocks.emplace(lock->GetLockName(), lock).second)("process_id", lock->GetLockName());
 }
 
 void TManager::UnregisterLock(const TString& processId) {
@@ -13,17 +14,17 @@ void TManager::UnregisterLock(const TString& processId) {
 
 std::optional<TString> TManager::IsLocked(const TPortionInfo& portion) const {
     for (auto&& i : ProcessLocks) {
-        if (i.second->IsLocked(portion)) {
-            return i.first;
+        if (auto lockName = i.second->IsLocked(portion)) {
+            return lockName;
         }
     }
     return {};
 }
 
 std::optional<TString> TManager::IsLocked(const TGranuleMeta& granule) const {
     for (auto&& i : ProcessLocks) {
-        if (i.second->IsLocked(granule)) {
-            return i.first;
+        if (auto lockName = i.second->IsLocked(granule)) {
+            return lockName;
         }
     }
     return {};
diff --git a/ydb/core/tx/columnshard/data_locks/manager/manager.h b/ydb/core/tx/columnshard/data_locks/manager/manager.h
@@ -12,10 +12,10 @@ class TManager {
 public:
     TManager() = default;
 
-    void RegisterLock(const TString& processId, const std::shared_ptr<ILock>& lock);
+    void RegisterLock(const std::shared_ptr<ILock>& lock);
     template <class TLock, class ...Args>
-    void RegisterLock(const TString& processId, Args&&... args) {
-        RegisterLock(processId, std::make_shared<TLock>(args...));
+    void RegisterLock(Args&&... args) {
+        RegisterLock(std::make_shared<TLock>(args...));
     }
     void UnregisterLock(const TString& processId);
     std::optional<TString> IsLocked(const TPortionInfo& portion) const;
diff --git a/ydb/core/tx/columnshard/engines/changes/abstract/abstract.cpp b/ydb/core/tx/columnshard/engines/changes/abstract/abstract.cpp
@@ -73,7 +73,7 @@ void TColumnEngineChanges::Abort(NColumnShard::TColumnShard& self, TChangesFinis
 }
 
 void TColumnEngineChanges::Start(NColumnShard::TColumnShard& self) {
-    self.DataLocksManager->RegisterLock(TypeString() + "::" + GetTaskIdentifier(), BuildDataLock());
+    self.DataLocksManager->RegisterLock(BuildDataLock());
     Y_ABORT_UNLESS(Stage == EStage::Created);
     DoStart(self);
     Stage = EStage::Started;
diff --git a/ydb/core/tx/columnshard/engines/changes/cleanup.h b/ydb/core/tx/columnshard/engines/changes/cleanup.h
@@ -28,7 +28,7 @@ class TCleanupColumnEngineChanges: public TColumnEngineChanges {
         return 0;
     }
     virtual std::shared_ptr<NDataLocks::ILock> DoBuildDataLock() const override {
-        return std::make_shared<NDataLocks::TListPortionsLock>(PortionsToDrop);
+        return std::make_shared<NDataLocks::TListPortionsLock>(TypeString() + "::" + GetTaskIdentifier(), PortionsToDrop);
     }
 
 public:
diff --git a/ydb/core/tx/columnshard/engines/changes/compaction.h b/ydb/core/tx/columnshard/engines/changes/compaction.h
@@ -25,7 +25,7 @@ class TCompactColumnEngineChanges: public TChangesWithAppend {
         NeedGranuleStatusProvide = false;
     }
     virtual std::shared_ptr<NDataLocks::ILock> DoBuildDataLockImpl() const override {
-        return std::make_shared<NDataLocks::TListPortionsLock>(SwitchedPortions);
+        return std::make_shared<NDataLocks::TListPortionsLock>(TypeString() + "::" + GetTaskIdentifier(), SwitchedPortions);
     }
 
 public:
diff --git a/ydb/core/tx/columnshard/engines/changes/ttl.h b/ydb/core/tx/columnshard/engines/changes/ttl.h
@@ -62,7 +62,7 @@ class TTTLColumnEngineChanges: public TChangesWithAppend {
         const auto pred = [](const TPortionForEviction& p) {
             return p.GetPortionInfo().GetAddress();
         };
-        return std::make_shared<NDataLocks::TListPortionsLock>(PortionsToEvict, pred);
+        return std::make_shared<NDataLocks::TListPortionsLock>(TypeString() + "::" + GetTaskIdentifier(), PortionsToEvict, pred);
     }
 public:
     class TMemoryPredictorSimplePolicy: public IMemoryPredictor {
diff --git a/ydb/core/tx/columnshard/engines/changes/with_appended.h b/ydb/core/tx/columnshard/engines/changes/with_appended.h
@@ -28,8 +28,13 @@ class TChangesWithAppend: public TColumnEngineChanges {
 
     virtual std::shared_ptr<NDataLocks::ILock> DoBuildDataLock() const override final {
         auto actLock = DoBuildDataLockImpl();
-        auto selfLock = std::make_shared<NDataLocks::TListPortionsLock>(PortionsToRemove);
-        return std::make_shared<NDataLocks::TCompositeLock>(std::vector<std::shared_ptr<NDataLocks::ILock>>({actLock, selfLock}));
+        if (actLock) {
+            auto selfLock = std::make_shared<NDataLocks::TListPortionsLock>(TypeString() + "::" + GetTaskIdentifier() + "::REMOVE", PortionsToRemove);
+            return std::make_shared<NDataLocks::TCompositeLock>(TypeString() + "::" + GetTaskIdentifier(), std::vector<std::shared_ptr<NDataLocks::ILock>>({actLock, selfLock}));
+        } else {
+            auto selfLock = std::make_shared<NDataLocks::TListPortionsLock>(TypeString() + "::" + GetTaskIdentifier(), PortionsToRemove);
+            return selfLock;
+        }
     }
 
 public:

Original file line number	Diff line number	Diff line change
`@@ -8,28 +8,28 @@ class TCompositeLock: public ILock {`
`8`	`8`	`using TBase = ILock;`
`9`	`9`	`std::vector<std::shared_ptr<ILock>> Locks;`
`10`	`10`	`protected:`
`11`		`- virtual bool DoIsLocked(const TPortionInfo& portion) const override {`
	`11`	`+ virtual std::optional<TString> DoIsLocked(const TPortionInfo& portion) const override {`
`12`	`12`	`for (auto&& i : Locks) {`
`13`		`- if (i->IsLocked(portion)) {`
`14`		`- return true;`
	`13`	`+ if (auto lockName = i->IsLocked(portion)) {`
	`14`	`+ return lockName;`
`15`	`15`	`}`
`16`	`16`	`}`
`17`		`- return false;`
	`17`	`+ return {};`
`18`	`18`	`}`
`19`		`- virtual bool DoIsLocked(const TGranuleMeta& granule) const override {`
	`19`	`+ virtual std::optional<TString> DoIsLocked(const TGranuleMeta& granule) const override {`
`20`	`20`	`for (auto&& i : Locks) {`
`21`		`- if (i->IsLocked(granule)) {`
`22`		`- return true;`
	`21`	`+ if (auto lockName = i->IsLocked(granule)) {`
	`22`	`+ return lockName;`
`23`	`23`	`}`
`24`	`24`	`}`
`25`		`- return false;`
	`25`	`+ return {};`
`26`	`26`	`}`
`27`	`27`	`bool DoIsEmpty() const override {`
`28`	`28`	`return Locks.empty();`
`29`	`29`	`}`
`30`	`30`	`public:`
`31`		`- TCompositeLock(const std::vector<std::shared_ptr<ILock>>& locks, const bool readOnly = false)`
`32`		`- : TBase(readOnly)`
	`31`	`+ TCompositeLock(const TString& lockName, const std::vector<std::shared_ptr<ILock>>& locks, const bool readOnly = false)`
	`32`	`+ : TBase(lockName, readOnly)`
`33`	`33`	`{`
`34`	`34`	`for (auto&& l : locks) {`
`35`	`35`	`if (!l \|\| l->IsEmpty()) {`
`@@ -39,8 +39,8 @@ class TCompositeLock: public ILock {`
`39`	`39`	`}`
`40`	`40`	`}`
`41`	`41`
`42`		`- TCompositeLock(std::initializer_list<std::shared_ptr<ILock>> locks, const bool readOnly = false)`
`43`		`- : TBase(readOnly)`
	`42`	`+ TCompositeLock(const TString& lockName, std::initializer_list<std::shared_ptr<ILock>> locks, const bool readOnly = false)`
	`43`	`+ : TBase(lockName, readOnly)`
`44`	`44`	`{`
`45`	`45`	`for (auto&& l : locks) {`
`46`	`46`	`if (!l \|\| l->IsEmpty()) {`
Original file line number	Diff line number	Diff line change
`@@ -3,8 +3,9 @@`
`3`	`3`
`4`	`4`	`namespace NKikimr::NOlap::NDataLocks {`
`5`	`5`
`6`		`-void TManager::RegisterLock(const TString& processId, const std::shared_ptr<ILock>& lock) {`
`7`		`- AFL_VERIFY(ProcessLocks.emplace(processId, lock).second)("process_id", processId);`
	`6`	`+void TManager::RegisterLock(const std::shared_ptr<ILock>& lock) {`
	`7`	`+ AFL_VERIFY(lock);`
	`8`	`+ AFL_VERIFY(ProcessLocks.emplace(lock->GetLockName(), lock).second)("process_id", lock->GetLockName());`
`8`	`9`	`}`
`9`	`10`
`10`	`11`	`void TManager::UnregisterLock(const TString& processId) {`
`@@ -13,17 +14,17 @@ void TManager::UnregisterLock(const TString& processId) {`
`13`	`14`
`14`	`15`	`std::optional<TString> TManager::IsLocked(const TPortionInfo& portion) const {`
`15`	`16`	`for (auto&& i : ProcessLocks) {`
`16`		`- if (i.second->IsLocked(portion)) {`
`17`		`- return i.first;`
	`17`	`+ if (auto lockName = i.second->IsLocked(portion)) {`
	`18`	`+ return lockName;`
`18`	`19`	`}`
`19`	`20`	`}`
`20`	`21`	`return {};`
`21`	`22`	`}`
`22`	`23`
`23`	`24`	`std::optional<TString> TManager::IsLocked(const TGranuleMeta& granule) const {`
`24`	`25`	`for (auto&& i : ProcessLocks) {`
`25`		`- if (i.second->IsLocked(granule)) {`
`26`		`- return i.first;`
	`26`	`+ if (auto lockName = i.second->IsLocked(granule)) {`
	`27`	`+ return lockName;`
`27`	`28`	`}`
`28`	`29`	`}`
`29`	`30`	`return {};`
Original file line number	Diff line number	Diff line change
`@@ -73,7 +73,7 @@ void TColumnEngineChanges::Abort(NColumnShard::TColumnShard& self, TChangesFinis`
`73`	`73`	`}`
`74`	`74`
`75`	`75`	`void TColumnEngineChanges::Start(NColumnShard::TColumnShard& self) {`
`76`		`- self.DataLocksManager->RegisterLock(TypeString() + "::" + GetTaskIdentifier(), BuildDataLock());`
	`76`	`+ self.DataLocksManager->RegisterLock(BuildDataLock());`
`77`	`77`	`Y_ABORT_UNLESS(Stage == EStage::Created);`
`78`	`78`	`DoStart(self);`
`79`	`79`	`Stage = EStage::Started;`
Original file line number	Diff line number	Diff line change
`@@ -28,7 +28,7 @@ class TCleanupColumnEngineChanges: public TColumnEngineChanges {`
`28`	`28`	`return 0;`
`29`	`29`	`}`
`30`	`30`	`virtual std::shared_ptr<NDataLocks::ILock> DoBuildDataLock() const override {`
`31`		`- return std::make_shared<NDataLocks::TListPortionsLock>(PortionsToDrop);`
	`31`	`+ return std::make_shared<NDataLocks::TListPortionsLock>(TypeString() + "::" + GetTaskIdentifier(), PortionsToDrop);`
`32`	`32`	`}`
`33`	`33`
`34`	`34`	`public:`
Original file line number	Diff line number	Diff line change
`@@ -25,7 +25,7 @@ class TCompactColumnEngineChanges: public TChangesWithAppend {`
`25`	`25`	`NeedGranuleStatusProvide = false;`
`26`	`26`	`}`
`27`	`27`	`virtual std::shared_ptr<NDataLocks::ILock> DoBuildDataLockImpl() const override {`
`28`		`- return std::make_shared<NDataLocks::TListPortionsLock>(SwitchedPortions);`
	`28`	`+ return std::make_shared<NDataLocks::TListPortionsLock>(TypeString() + "::" + GetTaskIdentifier(), SwitchedPortions);`
`29`	`29`	`}`
`30`	`30`
`31`	`31`	`public:`
Original file line number	Diff line number	Diff line change
`@@ -62,7 +62,7 @@ class TTTLColumnEngineChanges: public TChangesWithAppend {`
`62`	`62`	`const auto pred = [](const TPortionForEviction& p) {`
`63`	`63`	`return p.GetPortionInfo().GetAddress();`
`64`	`64`	`};`
`65`		`- return std::make_shared<NDataLocks::TListPortionsLock>(PortionsToEvict, pred);`
	`65`	`+ return std::make_shared<NDataLocks::TListPortionsLock>(TypeString() + "::" + GetTaskIdentifier(), PortionsToEvict, pred);`
`66`	`66`	`}`
`67`	`67`	`public:`
`68`	`68`	`class TMemoryPredictorSimplePolicy: public IMemoryPredictor {`