JBTM-4038 JGroupsSlots and JGroupsRaftSlots benchmarks#239
Conversation
📝 WalkthroughWalkthroughAdds JGroups and JGroups Raft configurations, Maven dependency wiring, three JMH slot-store benchmarks, transaction rollback cleanup, and updates benchmark CSV row-count validation for the expanded benchmark set. ChangesJGroups benchmark support
Sequence Diagram(s)sequenceDiagram
participant JMHRunner
participant RaftChannels
participant JGroupsRaftSlots
participant Leader
participant Follower
JMHRunner->>RaftChannels: create and connect three channels
RaftChannels->>RaftChannels: elect leader
JMHRunner->>JGroupsRaftSlots: initialize node slot stores
Leader->>Follower: replicate pre-populated slots
JMHRunner->>Follower: readFromFollower()
JMHRunner->>Leader: writeToLeader()
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/scripts/benchmark.sh:
- Line 113: Update BM5’s JMH selector pattern so it matches the newly added
JGroupsSlotsBenchmark and JGroupsRaftSlotsBenchmark benchmarks, including their
testJGroupsSlotsStore and testJGroupsRaftSlotsStore methods, while retaining the
existing StoreBenchmark matches and expected count of 7.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6999bbdf-b1be-4c03-ae87-b1357153804c
📒 Files selected for processing (6)
.github/scripts/benchmark.shnarayana/ArjunaJTA/jta/etc/jgroups-raft.xmlnarayana/ArjunaJTA/jta/etc/jgroups.xmlnarayana/ArjunaJTA/jta/pom.xmlnarayana/ArjunaJTA/jta/tests/classes/com/arjuna/ats/jta/xa/performance/JGroupsRaftSlotsBenchmark.javanarayana/ArjunaJTA/jta/tests/classes/com/arjuna/ats/jta/xa/performance/JGroupsSlotsBenchmark.java
3a97f23 to
c984b72
Compare
d21b12d to
c0a91a7
Compare
…uster of JGroupsRaftSlots (using SHARED_LOOPBACK)
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/scripts/benchmark.sh:
- Line 113: Update the BM5 benchmark selector configuration to use a lower bound
of 9 instead of 8, ensuring all four added StoreBenchmark methods are required
while preserving the existing selector.
In
`@narayana/ArjunaJTA/jta/tests/classes/com/arjuna/ats/jta/xa/performance/JGroupsRaftSlotsStoreBenchmark.java`:
- Around line 90-115: Remove the stale “configure 3 node cluster” TODO from the
setup method in JGroupsRaftSlotsStoreBenchmark, since
JGroupsRaftClusterStoreBenchmark now covers that configuration. Leave the
existing single-node benchmark setup unchanged.
In
`@narayana/ArjunaJTA/jta/tests/classes/com/arjuna/ats/jta/xa/performance/JTAStoreBase.java`:
- Around line 113-127: Update the transaction setup and cleanup around
tm.begin() in JTAStoreBase so rollback occurs only when this method successfully
started the transaction. Track that state with a local transactionStarted flag,
set it after begin succeeds, and guard both the catch-path rollback and
finally-block rollback with it, preserving suppressed-exception handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5676c249-55bc-4a74-9f89-97b5e1e0fa1b
📒 Files selected for processing (8)
.github/scripts/benchmark.shnarayana/ArjunaJTA/jta/etc/jgroups-raft.xmlnarayana/ArjunaJTA/jta/etc/jgroups.xmlnarayana/ArjunaJTA/jta/pom.xmlnarayana/ArjunaJTA/jta/tests/classes/com/arjuna/ats/jta/xa/performance/JGroupsRaftClusterStoreBenchmark.javanarayana/ArjunaJTA/jta/tests/classes/com/arjuna/ats/jta/xa/performance/JGroupsRaftSlotsStoreBenchmark.javanarayana/ArjunaJTA/jta/tests/classes/com/arjuna/ats/jta/xa/performance/JGroupsSlotsStoreBenchmark.javanarayana/ArjunaJTA/jta/tests/classes/com/arjuna/ats/jta/xa/performance/JTAStoreBase.java
| BM4d="ArjunaJTA/jta io.narayana.perf.product.AtomikosComparison.* 1" | ||
| #ArjunaJTA/jta/tests/classes/io/narayana/perf/product/ProductComparison.java | ||
| BM5="ArjunaJTA/jta com.arjuna.ats.jta.xa.performance.*StoreBenchmark.* 5" | ||
| BM5="ArjunaJTA/jta com.arjuna.ats.jta.xa.performance.*StoreBenchmark.* 8" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Set the lower bound to 9.
This selector now matches four added benchmark methods, so the prior lower bound of 5 should become 9. At 8, CI will still pass if any one new benchmark result is missing.
-BM5="ArjunaJTA/jta com.arjuna.ats.jta.xa.performance.*StoreBenchmark.* 8"
+BM5="ArjunaJTA/jta com.arjuna.ats.jta.xa.performance.*StoreBenchmark.* 9"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| BM5="ArjunaJTA/jta com.arjuna.ats.jta.xa.performance.*StoreBenchmark.* 8" | |
| BM5="ArjunaJTA/jta com.arjuna.ats.jta.xa.performance.*StoreBenchmark.* 9" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/scripts/benchmark.sh at line 113, Update the BM5 benchmark selector
configuration to use a lower bound of 9 instead of 8, ensuring all four added
StoreBenchmark methods are required while preserving the existing selector.
| @Setup(Level.Trial) | ||
| @BeforeClass | ||
| public static void setup() throws CoreEnvironmentBeanException { | ||
| JTAStoreBase.setup(SlotStoreAdaptor.class.getName()); | ||
| JGroupsRaftStoreEnvironmentBean configBean = BeanPopulator.getDefaultInstance(JGroupsRaftStoreEnvironmentBean.class); | ||
| BeanPopulator.getDefaultInstance(SlotStoreEnvironmentBean.class).setBackingSlotsClassName(JGroupsRaftSlots.class.getName()); | ||
| int threadCount = getThreadCountFromProperties(THREADS); | ||
|
|
||
| // JGroups configuration | ||
| configBean.setJGroupsConfigFileName("jgroups-raft.xml"); | ||
| configBean.setNodeAddress("raft-benchmark-node"); // TODO configure 3 node cluster | ||
| configBean.setClusterName("jgroups-raft-benchmark-" + System.currentTimeMillis()); | ||
|
|
||
| // Note: Using default key generator (Uid-based) for single-node benchmark | ||
|
|
||
| // Raft configuration (single-node cluster for benchmark) | ||
| configBean.setRaftMembers("raft-benchmark-node"); // Single node | ||
| configBean.setRaftLogFsync(true); // durable writes for fault tolerance | ||
| configBean.setAllowDirtyReads(false); | ||
|
|
||
| // Set the slot size, making sure it's a sensible multiple | ||
| configBean.setNumberOfSlots(roundUp(256, threadCount)); | ||
| configBean.setBackingSlotsClassName(JGroupsRaftSlots.class.getName()); | ||
|
|
||
| cleanStore(Paths.get(configBean.getStoreDir()).toFile()); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Stale TODO — 3-node cluster is now covered by JGroupsRaftClusterStoreBenchmark.
The // TODO configure 3 node cluster comment predates (or overlaps with) JGroupsRaftClusterStoreBenchmark, which already stands up a 3-node Raft cluster in this same PR. Update or remove the TODO so readers aren't pointed at unfinished work that has, in fact, been done elsewhere.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@narayana/ArjunaJTA/jta/tests/classes/com/arjuna/ats/jta/xa/performance/JGroupsRaftSlotsStoreBenchmark.java`
around lines 90 - 115, Remove the stale “configure 3 node cluster” TODO from the
setup method in JGroupsRaftSlotsStoreBenchmark, since
JGroupsRaftClusterStoreBenchmark now covers that configuration. Leave the
existing single-node benchmark setup unchanged.
| log.warn("JTAStoreTests#jtaTest%n", e); | ||
| try { | ||
| tm.getTransaction().rollback(); | ||
| } catch (Exception suppressed) { | ||
| e.addSuppressed(suppressed); | ||
| } | ||
| throw e; | ||
| } finally { | ||
| Transaction txn = tm.getTransaction(); | ||
| if (txn != null) { | ||
| try { | ||
| txn.rollback(); | ||
| } catch (Exception ignored) { | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
sed -n '1,220p' narayana/ArjunaJTA/jta/tests/classes/com/arjuna/ats/jta/xa/performance/JTAStoreBase.javaRepository: jbosstm/performance
Length of output: 4869
🏁 Script executed:
grep -Rni "class JTAStoreBase\|void jtaTest\|tm.begin\|getTransaction().rollback" narayana/ArjunaJTA/jta/tests/classes/com/arjuna/ats/jta/xa/performance/JTAStoreBase.javaRepository: jbosstm/performance
Length of output: 268
🏁 Script executed:
python3 - <<'PY'
from pathlib import Path
p = Path('narayana/ArjunaJTA/jta/tests/classes/com/arjuna/ats/jta/xa/performance/JTAStoreBase.java')
for i, line in enumerate(p.read_text().splitlines(), 1):
if 90 <= i <= 150:
print(f"{i:4}: {line}")
PYRepository: jbosstm/performance
Length of output: 1821
🌐 Web query:
Jakarta Transaction TransactionManager.begin throws NotSupportedException existing transaction getTransaction rollback semantics caller-owned transaction
💡 Result:
In the Jakarta Transactions API, the TransactionManager.begin method throws a NotSupportedException when the calling thread is already associated with an active transaction and the underlying transaction manager implementation does not support nested transactions [1][2]. By design, the standard Jakarta EE transaction manager does not support nested transactions [3]. If a thread is already associated with a transaction, the manager requires that the existing transaction be completed (committed or rolled back) before a new one can be started [3]. Regarding caller-owned transaction and rollback semantics: 1. Ownership and Association: The Jakarta Transactions specification associates transaction context with the calling thread [2][4]. When a thread is associated with a transaction, that transaction is considered the active context for that thread [2]. 2. Rollback Semantics: The TransactionManager.rollback method is specifically designed to roll back the transaction currently associated with the calling thread [2][5]. Once the rollback completes, the thread is no longer associated with that transaction [2][5]. 3. External Control: Because Jakarta Transactions operates based on thread association, if a "caller-owned" transaction (e.g., one started by a parent component) is active on the current thread, any subsequent code—including nested calls—operates within that same transaction context unless it is explicitly suspended [2][4]. A caller cannot typically "own" a transaction in a way that prevents the current thread from interacting with it if that transaction is currently associated with that thread [2]. If the calling code needs to prevent interference or handle transaction boundaries differently, it must manage suspension or ensure that the existing transaction is properly concluded before initiating a new one [2][3].
Citations:
- 1: https://jakarta.ee/specifications/platform/9/apidocs/jakarta/transaction/transactionmanager
- 2: https://jakarta.ee/specifications/transactions/2.0/jakarta-transactions-spec-2.0
- 3: https://jakarta.ee/learn/jakartaee-tutorial/current/supporttechs/transactions/transactions.html
- 4: https://jakarta.ee/specifications/transactions/2.0/jakarta-transactions-spec-2.0.html
- 5: https://jakarta.ee/specifications/webprofile/9.1/apidocs/jakarta/transaction/transactionmanager
🏁 Script executed:
rg -n "jtaTest\(" narayana/ArjunaJTA/jta/tests/classes -g '!**/target/**'Repository: jbosstm/performance
Length of output: 1912
🏁 Script executed:
python3 - <<'PY'
from pathlib import Path
for p in Path('narayana/ArjunaJTA/jta/tests/classes').rglob('*.java'):
txt = p.read_text(errors='ignore')
if 'jtaTest(' in txt and p.name != 'JTAStoreBase.java':
for i, line in enumerate(txt.splitlines(), 1):
if 'jtaTest(' in line:
print(f"{p}:{i}:{line.strip()}")
PYRepository: jbosstm/performance
Length of output: 1594
🏁 Script executed:
sed -n '1,220p' narayana/ArjunaJTA/jta/tests/classes/com/arjuna/ats/jta/xa/performance/JTAStoreBase.javaRepository: jbosstm/performance
Length of output: 4869
🏁 Script executed:
python3 - <<'PY'
from pathlib import Path
for p in Path('narayana/ArjunaJTA/jta/tests/classes').rglob('*.java'):
txt = p.read_text(errors='ignore').splitlines()
for i, line in enumerate(txt, 1):
if 'jtaTest(' in line and 'JTAStoreBase' not in str(p):
print(f"{p}:{i}:{line.strip()}")
PYRepository: jbosstm/performance
Length of output: 1594
🌐 Web query:
Jakarta Transactions TransactionManager.getTransaction after begin throws NotSupportedException existing associated transaction remains active rollback current thread
💡 Result:
In the Jakarta Transactions (JTA) specification, the NotSupportedException is thrown by the TransactionManager.begin method when a thread is already associated with a transaction and the underlying transaction manager implementation does not support nested transactions [1][2][3]. When this exception is thrown, the existing transaction associated with the thread remains active and is not automatically rolled back by the TransactionManager.begin call [2][3][4]. The exception indicates that the request to start a new transaction cannot be fulfilled [5]; it does not imply that the current transactional context has been invalidated or ended [2][3]. To handle this situation, the developer is responsible for managing the state of the existing transaction. Since the original transaction remains active, you may choose to: 1. Continue using the existing transaction (if the logic allows). 2. Explicitly roll back the existing transaction using the TransactionManager.rollback method before attempting further operations, if the failure to start a new nested transaction renders the current unit of work invalid [6][7]. The TransactionManager.getTransaction method will continue to return the Transaction object representing the pre-existing, still-active transaction context [1][8][9]. If you require a new, independent transaction context, you must ensure that the current transaction is completed (committed or rolled back) and dissociated from the thread (typically via suspension or completion) before calling begin again [2][3][6].
Citations:
- 1: https://jakarta.ee/specifications/transactions/2.0/apidocs/jakarta/transaction/transactionmanager
- 2: https://jakarta.ee/specifications/transactions/2.0/jakarta-transactions-spec-2.0
- 3: https://github.com/eclipse-ee4j/jta-api/blob/master/spec/src/main/asciidoc/Transactions.adoc
- 4: https://jakarta.ee/specifications/transactions/2.0/jakarta-transactions-spec-2.0.html
- 5: https://jakarta.ee/specifications/transactions/2.0/apidocs/jakarta/transaction/notsupportedexception
- 6: https://grokipedia.com/page/jakarta_transactions
- 7: http://www.datadisk.co.uk/html_docs/ejb/ejb3_transactions.htm
- 8: https://jakarta.ee/specifications/platform/9/apidocs/jakarta/transaction/transactionmanager
- 9: https://jakarta.ee/specifications/platform/10/apidocs/jakarta/transaction/transactionmanager
🏁 Script executed:
python3 - <<'PY'
from pathlib import Path
base = Path('narayana/ArjunaJTA/jta/tests/classes/com/arjuna/ats/jta/xa/performance')
for p in base.rglob('*.java'):
txt = p.read_text(errors='ignore')
if p.name != 'JTAStoreBase.java' and ('extends JTAStoreBase' in txt or 'jtaTest()' in txt):
print(f"\n{p}")
for i, line in enumerate(txt.splitlines(), 1):
if 'extends JTAStoreBase' in line or 'jtaTest()' in line:
print(f"{i}: {line}")
PYRepository: jbosstm/performance
Length of output: 2110
Only roll back the transaction started here
If tm.begin() throws NotSupportedException because the thread is already associated with a transaction, tm.getTransaction() still returns that pre-existing transaction. Both cleanup paths can then abort work owned by the caller; gate rollback on a local transactionStarted flag.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@narayana/ArjunaJTA/jta/tests/classes/com/arjuna/ats/jta/xa/performance/JTAStoreBase.java`
around lines 113 - 127, Update the transaction setup and cleanup around
tm.begin() in JTAStoreBase so rollback occurs only when this method successfully
started the transaction. Track that state with a local transactionStarted flag,
set it after begin succeeds, and guard both the catch-path rollback and
finally-block rollback with it, preserving suppressed-exception handling.
1 participant
https://redhat.atlassian.net/browse/JBTM-4038
NARAYANA_REPO=mmusgrov
NARAYANA_BRANCH=JBTM-4038
NY_BRANCH=JBTM-4038
THREAD_COUNTS=1 24 240
COMPARE_STORES=y
COMPARE_IMPLEMENTATIONS=n
COMPARE_TRANSPORTS=n
COMPARE_JOURNAL_PARAMETERS=n
JDK25