-
Notifications
You must be signed in to change notification settings - Fork 4
786 Fix re-aquisition of expired locks when JDBC or DynamoDB is used as locking mechanism #787
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -24,7 +24,7 @@ import za.co.absa.pramen.core.fixtures.RelationalDbFixture | |
| import za.co.absa.pramen.core.lock.{TokenLockBase, TokenLockJdbc, TokenLockRegistry} | ||
| import za.co.absa.pramen.core.rdb.{PramenDb, RdbJdbc} | ||
| import za.co.absa.pramen.core.reader.model.JdbcConfig | ||
| import za.co.absa.pramen.core.utils.UsingUtils | ||
| import za.co.absa.pramen.core.utils.{SlickUtils, UsingUtils} | ||
|
|
||
| import scala.concurrent.duration._ | ||
|
|
||
|
|
@@ -82,6 +82,23 @@ class TokenLockJdbcSuite extends AnyWordSpec with RelationalDbFixture with Befor | |
| lock2.release() | ||
| } | ||
|
|
||
| "allow releasing locks for other owners if requested" in { | ||
| val lock1 = getLock("token1") | ||
| val lock2 = getLock("token2") | ||
|
|
||
| assert(lock1.tryAcquire()) | ||
| assert(lock2.tryAcquire()) | ||
|
|
||
| lock1.asInstanceOf[TokenLockJdbc].releaseGuardLock(evenNonOwned = false) | ||
| lock2.asInstanceOf[TokenLockJdbc].releaseGuardLock(evenNonOwned = true) | ||
|
|
||
| val slickUtils = new SlickUtils(pramenDb.slickProfile) | ||
|
|
||
| val recordCount = slickUtils.executeCount(pramenDb.slickDb, pramenDb.lockTicketTable.records.length) | ||
|
|
||
| assert(recordCount == 0) | ||
|
Comment on lines
+92
to
+99
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -eu
printf '%s\n' '--- candidate files ---'
git ls-files 'pramen/core/src/main/scala/za/co/absa/pramen/core/lock/*' \
'pramen/core/src/test/scala/za/co/absa/pramen/core/tests/lock/*' | sort
printf '%s\n' '--- relevant declarations and call sites ---'
rg -n -C 5 \
'releaseGuardLock|acquireGuardLock|expired|token1|token2|evenNonOwned|recordCount' \
pramen/core/src/main/scala/za/co/absa/pramen/core/lock \
pramen/core/src/test/scala/za/co/absa/pramen/core/tests/lock/TokenLockJdbcSuite.scala \
pramen/core/src/test/scala/za/co/absa/pramen/core/tests/lock/TokenLockSQLiteSuite.scalaRepository: AbsaOSS/pramen Length of output: 36044 🏁 Script executed: #!/bin/bash
set -eu
printf '%s\n' '--- TokenLockJdbc implementation ---'
wc -l pramen/core/src/main/scala/za/co/absa/pramen/core/lock/TokenLockJdbc.scala
sed -n '1,155p' pramen/core/src/main/scala/za/co/absa/pramen/core/lock/TokenLockJdbc.scala
printf '%s\n' '--- shared base and test setup ---'
sed -n '1,175p' pramen/core/src/main/scala/za/co/absa/pramen/core/lock/TokenLockBase.scala
sed -n '1,135p' pramen/core/src/test/scala/za/co/absa/pramen/core/tests/lock/TokenLockJdbcSuite.scala
sed -n '1,145p' pramen/core/src/test/scala/za/co/absa/pramen/core/tests/lock/TokenLockSQLiteSuite.scala
printf '%s\n' '--- factory and ownership definitions ---'
rg -n -C 6 'def getLock|new TokenLockJdbc|owner|escapedToken|TICKETS_HARD_EXPIRE_DAYS|def release' \
pramen/core/src/main/scala \
pramen/core/src/test/scala/za/co/absa/pramen/core/tests/lockRepository: AbsaOSS/pramen Length of output: 50370 🏁 Script executed: #!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
import re
jdbc = Path("pramen/core/src/test/scala/za/co/absa/pramen/core/tests/lock/TokenLockJdbcSuite.scala").read_text()
sqlite = Path("pramen/core/src/test/scala/za/co/absa/pramen/core/tests/lock/TokenLockSQLiteSuite.scala").read_text()
impl = Path("pramen/core/src/main/scala/za/co/absa/pramen/core/lock/TokenLockJdbc.scala").read_text()
assert re.search(r"protected\s+def\s+releaseGuardLock\s*\(\s*evenNonOwned:\s*Boolean\s*\)", impl)
assert "package za.co.absa.pramen.core.lock" in impl
assert "package za.co.absa.pramen.core.tests.lock" in jdbc
assert "package za.co.absa.pramen.core.tests.lock" in sqlite
for name, text, expected_line in (
("TokenLockJdbcSuite.scala", jdbc, "releaseGuardLock(evenNonOwned = false)"),
("TokenLockSQLiteSuite.scala", sqlite, "releaseGuardLock(evenNonOwned = false)"),
):
block = re.search(
r'"allow releasing locks for other owners if requested"\s+in\s*\{(.*?)\n\s*\}',
text,
re.S,
)
assert block, name
body = block.group(1)
tokens = re.findall(r'getLock\("([^"]+)"\)', body)
calls = re.findall(r'releaseGuardLock\(evenNonOwned\s*=\s*(true|false)\)', body)
print(f"{name}: test tokens={tokens}, cleanup flags={calls}")
assert tokens == ["token1", "token2"]
assert calls == ["false", "true"]
assert "new TokenLockJdbc" not in body
assert "token1" not in body.split(expected_line, 1)[-1] or True
print("TokenLockJdbc.releaseGuardLock is protected.")
print("Both tests are in za.co.absa.pramen.core.tests.lock, not za.co.absa.pramen.core.lock.")
print("Both tests invoke cleanup on different tokens, so evenNonOwned=true is never tested against another owner of the same token.")
PYRepository: AbsaOSS/pramen Length of output: 240 🏁 Script executed: #!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
import re
base = Path("pramen/core/src/main/scala/za/co/absa/pramen/core/lock/TokenLockBase.scala").read_text()
jdbc_impl = Path("pramen/core/src/main/scala/za/co/absa/pramen/core/lock/TokenLockJdbc.scala").read_text()
jdbc_test = Path("pramen/core/src/test/scala/za/co/absa/pramen/core/tests/lock/TokenLockJdbcSuite.scala").read_text()
sqlite_test = Path("pramen/core/src/test/scala/za/co/absa/pramen/core/tests/lock/TokenLockSQLiteSuite.scala").read_text()
assert re.search(r"protected\s+def\s+releaseGuardLock\s*\(\s*evenNonOwned:\s*Boolean\s*\)", base)
assert re.search(r"override\s+def\s+releaseGuardLock\s*\(\s*evenNonOwned:\s*Boolean\s*\)", jdbc_impl)
for name, text in (("TokenLockJdbcSuite.scala", jdbc_test), ("TokenLockSQLiteSuite.scala", sqlite_test)):
block = re.search(
r'"allow releasing locks for other owners if requested"\s+in\s*\{(.*?)\n\s*\}',
text,
re.S,
)
assert block, name
body = block.group(1)
tokens = re.findall(r'getLock\("([^"]+)"\)', body)
flags = re.findall(r'releaseGuardLock\(evenNonOwned\s*=\s*(true|false)\)', body)
print(f"{name}: tokens={tokens}; cleanup flags={flags}")
assert tokens == ["token1", "token2"]
assert flags == ["false", "true"]
assert "token1" not in body.split("val lock1", 1)[-1].split("val lock2", 1)[0]
print("Base declaration: protected.")
print("TokenLockJdbc override: no protected modifier.")
print("Both tests use different tokens, so the true branch is not tested against a ticket owned by another instance for the same token.")
PY
printf '%s\n' '--- compiler availability (without compiling repository code) ---'
command -v scalac || true
scalac -version 2>/dev/null || trueRepository: AbsaOSS/pramen Length of output: 328 🌐 Web query:
💡 Result: In Scala, the rules for overriding class members are governed by the requirement that the overriding member's access visibility must be as broad as or broader than the overridden member's access visibility [1][2]. Specifically, you cannot make a member more restrictive when overriding it [3][4]. When overriding a protected member in Scala: 1. Widen Access (Public): You can override a protected member with a public member [3][4]. Because Scala does not have an explicit public keyword, omitting an access modifier effectively makes the member public [3]. 2. Maintain or Narrow Access: You can keep the member protected or, in some cases, apply a qualified access modifier (e.g., protected[this] or protected[package]), provided the resulting visibility is not more restrictive than the original protected scope [3][5]. It is important to note that access modifiers are not inherited automatically [3]. If you override a protected method and do not specify an access modifier, the overriding method will become public by default [3]. If your intention is to keep the method protected, you must explicitly include the protected keyword in the overriding definition [3][6]. Additionally, when overriding Java methods from Scala, the compiler performs strict checks to ensure the Scala implementation satisfies the binary compatibility requirements of the JVM [7]. In some cases, especially when interacting with Java, the compiler may enforce that the overriding method remains public if the base method was public, or may restrict certain widenings to ensure the generated bytecode is valid [8][7]. Summary of rules: - To make a protected method public: Simply override it without an access modifier [3]. - To keep a protected method protected: You must explicitly re-declare it as protected [3][6]. - Restriction: You cannot override a public method with a protected method, as this would be a restriction of access, which is prohibited [3][9][4]. Citations:
🏁 Script executed: #!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
import re
base = Path("pramen/core/src/main/scala/za/co/absa/pramen/core/lock/TokenLockBase.scala").read_text()
jdbc_impl = Path("pramen/core/src/main/scala/za/co/absa/pramen/core/lock/TokenLockJdbc.scala").read_text()
assert re.search(r"protected\s+def\s+releaseGuardLock\s*\(\s*evenNonOwned:\s*Boolean\s*\)", base)
assert re.search(r"override\s+def\s+releaseGuardLock\s*\(\s*evenNonOwned:\s*Boolean\s*\)", jdbc_impl)
for path in (
"pramen/core/src/test/scala/za/co/absa/pramen/core/tests/lock/TokenLockJdbcSuite.scala",
"pramen/core/src/test/scala/za/co/absa/pramen/core/tests/lock/TokenLockSQLiteSuite.scala",
):
text = Path(path).read_text()
block = re.search(
r'"allow releasing locks for other owners if requested"\s+in\s*\{(.*?)\n\s*\}',
text,
re.S,
)
assert block, path
body = block.group(1)
tokens = re.findall(r'getLock\("([^"]+)"\)', body)
flags = re.findall(r'releaseGuardLock\(evenNonOwned\s*=\s*(true|false)\)', body)
assert tokens == ["token1", "token2"], (path, tokens)
assert flags == ["false", "true"], (path, flags)
print(f"{path}: different tokens {tokens}; flags {flags}")
print("TokenLockBase.releaseGuardLock is protected.")
print("TokenLockJdbc.releaseGuardLock is public because its override omits protected.")
print("The current test does not exercise same-token, different-owner cleanup.")
PYRepository: AbsaOSS/pramen Length of output: 650 Test same-token ownership cleanup in both suites.
Create two 📍 Affects 2 files
🤖 Prompt for AI Agents |
||
| } | ||
|
|
||
| "lock pramen should constantly update lock ticket" in { | ||
| val lock1 = new TokenLockJdbc("token1", pramenDb.slickDb, pramenDb.slickProfile) { | ||
| override val tokenExpiresSeconds = 2L | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Make expired-ticket removal conditional in the same database operation.
A process can read an expired ticket, while another process renews or replaces it before this unconditional deletion. The first process then deletes the new valid ticket and can acquire the token while the other process still considers itself the owner.
pramen/core/src/main/scala/za/co/absa/pramen/core/lock/TokenLockDynamoDb.scala#L81-L82: use a conditional delete that requiresexpires_atto remain expired.pramen/core/src/main/scala/za/co/absa/pramen/core/lock/TokenLockJdbc.scala#L59-L60: delete only a row whose token matches and whose expiry remains expired.pramen/core/src/main/scala/za/co/absa/pramen/core/lock/TokenLockMongoDb.scala#L70-L71: use adeleteOnefilter that requires the token and an expiredexpiresvalue.📍 Affects 3 files
pramen/core/src/main/scala/za/co/absa/pramen/core/lock/TokenLockDynamoDb.scala#L81-L82(this comment)pramen/core/src/main/scala/za/co/absa/pramen/core/lock/TokenLockJdbc.scala#L59-L60pramen/core/src/main/scala/za/co/absa/pramen/core/lock/TokenLockMongoDb.scala#L70-L71🤖 Prompt for AI Agents