Skip to content

Optimize UsageJobDaoImpl.updateJobSuccess to use direct UPDATE instead of row-level lock#13160

Draft
vishesh92 wants to merge 1 commit into
apache:4.22from
shapeblue:optimize-usagejob-success-method
Draft

Optimize UsageJobDaoImpl.updateJobSuccess to use direct UPDATE instead of row-level lock#13160
vishesh92 wants to merge 1 commit into
apache:4.22from
shapeblue:optimize-usagejob-success-method

Conversation

@vishesh92
Copy link
Copy Markdown
Member

Description

Replace lockRow + explicit transaction with a single update(jobId, jobForUpdate) call. All field values are parameters passed by the caller — nothing is read from the locked row.

The TransactionLegacy.open(USAGE_DB) is kept since this method explicitly targets the usage database. The txn.start/commit/rollback are removed since the single update() call is autocommit.

The lock was redundant because the usage server model is single-owner: one server processes one job at a time.

Types of changes

  • Breaking change (fix or feature that would cause existing functionality to change)
  • New feature (non-breaking change which adds functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • Enhancement (improves an existing feature and functionality)
  • Cleanup (Code refactoring and cleanup, that may add test cases)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

  • Major
  • Minor

Bug Severity

  • BLOCKER
  • Critical
  • Major
  • Minor
  • Trivial

Screenshots (if appropriate):

How Has This Been Tested?

Compilation and static analysis:

  • mvn -pl engine/schema -am compile -DskipTests — compiles clean
  • mvn -pl engine/schema checkstyle:check — passes
  • mvn -pl engine/schema test — all tests pass

Code-level verification:

  • The update(jobId, jobForUpdate) call generates a single UPDATE usage_job SET start_millis=?, end_millis=?, exec_time=?, start_date=?, end_date=?, success=? WHERE id=? — all columns written atomically in one statement
  • TransactionLegacy.open(USAGE_DB) is kept since this method explicitly targets the usage database connection. The txn.start()/txn.commit()/txn.rollback() are removed since the single update() call is autocommit
  • Confirmed lockRow no longer appears in the method
  • The original code read job.getId() from the locked row to pass to update() — but job.getId() always equals jobId (the parameter), since lockRow(jobId) fetches the row by that same ID. The new code uses jobId directly, eliminating the unnecessary indirection

Behavioral equivalence:

  • All six field values (startMillis, endMillis, execTime, startDate, endDate, success) are parameters passed by the caller — nothing is read from the locked row that feeds into the write
  • The usage server model is single-owner: one server processes one job at a time. Concurrent updates to the same job row do not occur by design, so the lock was defense-in-depth rather than a correctness requirement

…d of row-level lock

Replace lockRow + explicit transaction with a single update(jobId,
jobForUpdate) call. All field values are parameters passed by the
caller — nothing is read from the locked row.

The TransactionLegacy.open(USAGE_DB) is kept since this method
explicitly targets the usage database. The txn.start/commit/rollback
are removed since the single update() call is autocommit.

The lock was redundant because the usage server model is single-owner:
one server processes one job at a time.
@vishesh92
Copy link
Copy Markdown
Member Author

@blueorangutan package

@blueorangutan
Copy link
Copy Markdown

@vishesh92 a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

@codecov
Copy link
Copy Markdown

codecov Bot commented May 14, 2026

Codecov Report

❌ Patch coverage is 0% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 17.67%. Comparing base (a289bb0) to head (6c6aa4e).

Files with missing lines Patch % Lines
...main/java/com/cloud/usage/dao/UsageJobDaoImpl.java 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               4.22   #13160      +/-   ##
============================================
- Coverage     17.67%   17.67%   -0.01%     
+ Complexity    15792    15787       -5     
============================================
  Files          5922     5922              
  Lines        533123   533119       -4     
  Branches      65201    65201              
============================================
- Hits          94246    94239       -7     
+ Misses       428236   428234       -2     
- Partials      10641    10646       +5     
Flag Coverage Δ
uitests 3.69% <ø> (ø)
unittests 18.75% <0.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants