fix: update status of pending chunks in redis/local queue #3355
Conversation
WalkthroughAdds a configurable per-instance concurrency limit for machine-translation jobs, plugs it into the MT chunk processor, tightens execution-start checks in the progress manager and launcher to avoid requeueing/completing jobs incorrectly, and adds test helpers and a test validating the new limit. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Test as Test/TestUtil
participant Queue as ExecutionQueue
participant Launcher as BatchJobConcurrentLauncher
participant Progress as ProgressManager
participant Processor as MTChunkProcessor
participant DB as DB / BatchJobService
Test->>Queue: enqueue MACHINE_TRANSLATE job
Queue->>Launcher: take executionItem
Launcher->>DB: getJobDto(executionItem.jobId)
DB-->>Launcher: jobDto
Launcher->>Launcher: read maxPerJobConcurrency (cached from jobDto or BatchProperties)
Launcher->>DB: count running executions for job
DB-->>Launcher: runningCount
alt runningCount < maxPerJobConcurrency or max = -1
Launcher->>Progress: trySetExecutionRunning(executionId)
Progress->>DB: getExecutionState(executionId)
DB-->>Progress: state (completed? false)
Progress-->>Launcher: true
Launcher->>Processor: process chunks (coroutines)
Processor->>DB: update progress / store chunks
Processor-->>Launcher: processing completed
else runningCount >= maxPerJobConcurrency
Launcher->>DB: getJobDto(executionItem.jobId)
DB-->>Launcher: jobDto
alt jobDto.status.completed == true
Launcher-->>Queue: drop (do not requeue)
else
Launcher-->>Queue: requeue executionItem
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 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: 0
🧹 Nitpick comments (2)
backend/data/src/main/kotlin/io/tolgee/batch/processors/MachineTranslationChunkProcessor.kt (1)
50-59: Good documentation explaining the rationale for limiting MT concurrency to 1.Minor typo on line 53: missing space after comma in
"OpenAI,we"→ should be"OpenAI, we".🔎 Apply this diff to fix the typo:
- * calculate how many credits to charge the organization. However, if it were to be parallelized too much, + * calculate how many credits to charge the organization. However, if it were to be parallelized too much,Actually, the issue is on line 53:
- * only when we have the result from OpenAI,we are able to + * only when we have the result from OpenAI, we are able tobackend/app/src/test/kotlin/io/tolgee/batch/BatchJobTestUtil.kt (1)
47-47: Unnecessary import.
kotlin.applyis automatically imported as part of Kotlin's standard library and doesn't need an explicit import.🔎 Apply this diff to remove the unnecessary import:
-import kotlin.apply
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
backend/app/src/test/kotlin/io/tolgee/batch/AbstractBatchJobsGeneralTest.kt(1 hunks)backend/app/src/test/kotlin/io/tolgee/batch/BatchJobTestUtil.kt(3 hunks)backend/data/src/main/kotlin/io/tolgee/batch/BatchJobConcurrentLauncher.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/batch/ProgressManager.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/batch/processors/MachineTranslationChunkProcessor.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/model/batch/BatchJob.kt(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
backend/**/*.kt
📄 CodeRabbit inference engine (AGENTS.md)
backend/**/*.kt: After modifying JPA entities, run./gradlew diffChangeLogto generate Liquibase changelog entries (add--no-daemonflag if docker command not found)
Run./gradlew ktlintFormatbefore committing code
Files:
backend/app/src/test/kotlin/io/tolgee/batch/AbstractBatchJobsGeneralTest.ktbackend/data/src/main/kotlin/io/tolgee/batch/ProgressManager.ktbackend/app/src/test/kotlin/io/tolgee/batch/BatchJobTestUtil.ktbackend/data/src/main/kotlin/io/tolgee/model/batch/BatchJob.ktbackend/data/src/main/kotlin/io/tolgee/batch/BatchJobConcurrentLauncher.ktbackend/data/src/main/kotlin/io/tolgee/batch/processors/MachineTranslationChunkProcessor.kt
backend/**/*Test.kt
📄 CodeRabbit inference engine (AGENTS.md)
backend/**/*Test.kt: Use TestData classes for test setup with TestDataService for saving and cleaning test data in unit tests
Use.andAssertThatJsonfor testing API JSON responses in tests
Files:
backend/app/src/test/kotlin/io/tolgee/batch/AbstractBatchJobsGeneralTest.kt
**/*.{ts,tsx,js,jsx,kt,kts}
⚙️ CodeRabbit configuration file
As part of review, please check if the file follows 'The Stepdown Rule': The most important, high-level concepts should be at the top (like a newspaper headline and opening paragraph). Details should increase as you read downward. Functions should be ordered so that a caller appears before the functions it calls. When working with JavaScript components, we allow the main component to live at the bottom of the file as an exception to the rule.
Files:
backend/app/src/test/kotlin/io/tolgee/batch/AbstractBatchJobsGeneralTest.ktbackend/data/src/main/kotlin/io/tolgee/batch/ProgressManager.ktbackend/app/src/test/kotlin/io/tolgee/batch/BatchJobTestUtil.ktbackend/data/src/main/kotlin/io/tolgee/model/batch/BatchJob.ktbackend/data/src/main/kotlin/io/tolgee/batch/BatchJobConcurrentLauncher.ktbackend/data/src/main/kotlin/io/tolgee/batch/processors/MachineTranslationChunkProcessor.kt
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build frontend 🏗️
- GitHub Check: Frontend static check 🪲
- GitHub Check: Build backend 🏗️
🔇 Additional comments (6)
backend/data/src/main/kotlin/io/tolgee/batch/ProgressManager.kt (1)
44-46: LGTM! This is the core fix for the PR.When an
ExecutionStatealready exists (e.g., in PENDING status from a previous attempt), updating its status to RUNNING ensures the concurrent execution count is accurate. Without this, theRUNNINGcount check intrySetRunningStatewould undercount, allowing more concurrent executions thanmaxPerJobConcurrencypermits.backend/data/src/main/kotlin/io/tolgee/batch/BatchJobConcurrentLauncher.kt (1)
264-276: LGTM! The concurrency check logic is correct.The changes properly:
- Cache
maxPerJobConcurrencyto avoid repeated lookups- Short-circuit when unlimited (
-1)- Use the cached value in the final comparison
backend/data/src/main/kotlin/io/tolgee/model/batch/BatchJob.kt (1)
66-71: LGTM! Good documentation addition.The KDoc clearly explains the semantics of
maxPerJobConcurrency(-1 for unlimited) and references the relatedtrySetRunningState()method for additional context.backend/app/src/test/kotlin/io/tolgee/batch/AbstractBatchJobsGeneralTest.kt (1)
347-360: LGTM! Good test coverage for themaxPerJobConcurrencyfix.The test properly verifies:
- The configured concurrency limit (
assertAllowedMaxPerJobConcurrency)- Runtime enforcement (
assertMaxPerJobConcurrencyIsLessThanOrEqualTo)- Job completion and cleanup (
waitForJobSuccess,assertJobUnlocked)backend/app/src/test/kotlin/io/tolgee/batch/BatchJobTestUtil.kt (2)
452-474: LGTM! Well-structured test utility for MT jobs.The
runMtJobfunction follows the same pattern as other job runner utilities in this class.
476-493: LGTM! Concurrency assertion utilities are well-designed.The
assertMaxPerJobConcurrencyIsLessThanOrEqualTofunction polls until the queue is empty while asserting the concurrency limit on each iteration. If the limit is exceeded, the assertion will fail immediately rather than waiting for timeout.
JanCizmar
left a comment
JanCizmar
left a comment
There was a problem hiding this comment.
Great job! Thx ❤️
Please fix the PR name, so it fits the space 🙏
bdshadow
force-pushed
the
fix/respect-maxPerJobConcurrency
branch
from
81817f6 to
63616b2
Compare
bdshadow
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
backend/data/src/main/kotlin/io/tolgee/batch/BatchJobConcurrentLauncher.kt (1)
270-276: Consider renamingcounttorunningCountfor clarity.The variable name
countcould be more descriptive. Consider renaming it torunningCountorcurrentRunningCountto make the concurrency check more explicit.🔎 Proposed refactor
- val count = + val runningCount = it.values.count { executionState -> executionState.status == BatchJobChunkExecutionStatus.RUNNING } - if (count == 0) { + if (runningCount == 0) { return@trySetExecutionRunning true } - maxPerJobConcurrency > count + maxPerJobConcurrency > runningCount
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
backend/app/src/test/kotlin/io/tolgee/batch/AbstractBatchJobsGeneralTest.kt(1 hunks)backend/app/src/test/kotlin/io/tolgee/batch/BatchJobTestUtil.kt(3 hunks)backend/data/src/main/kotlin/io/tolgee/batch/BatchJobConcurrentLauncher.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/batch/ProgressManager.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/batch/processors/MachineTranslationChunkProcessor.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/model/batch/BatchJob.kt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- backend/app/src/test/kotlin/io/tolgee/batch/AbstractBatchJobsGeneralTest.kt
- backend/data/src/main/kotlin/io/tolgee/model/batch/BatchJob.kt
- backend/data/src/main/kotlin/io/tolgee/batch/processors/MachineTranslationChunkProcessor.kt
🧰 Additional context used
📓 Path-based instructions (2)
backend/**/*.kt
📄 CodeRabbit inference engine (AGENTS.md)
backend/**/*.kt: After modifying JPA entities, run./gradlew diffChangeLogto generate Liquibase changelog entries (add--no-daemonflag if docker command not found)
Run./gradlew ktlintFormatbefore committing code
Files:
backend/data/src/main/kotlin/io/tolgee/batch/ProgressManager.ktbackend/data/src/main/kotlin/io/tolgee/batch/BatchJobConcurrentLauncher.ktbackend/app/src/test/kotlin/io/tolgee/batch/BatchJobTestUtil.kt
**/*.{ts,tsx,js,jsx,kt,kts}
⚙️ CodeRabbit configuration file
As part of review, please check if the file follows 'The Stepdown Rule': The most important, high-level concepts should be at the top (like a newspaper headline and opening paragraph). Details should increase as you read downward. Functions should be ordered so that a caller appears before the functions it calls. When working with JavaScript components, we allow the main component to live at the bottom of the file as an exception to the rule.
Files:
backend/data/src/main/kotlin/io/tolgee/batch/ProgressManager.ktbackend/data/src/main/kotlin/io/tolgee/batch/BatchJobConcurrentLauncher.ktbackend/app/src/test/kotlin/io/tolgee/batch/BatchJobTestUtil.kt
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build frontend 🏗️
- GitHub Check: Build backend 🏗️
- GitHub Check: Frontend static check 🪲
🔇 Additional comments (3)
backend/data/src/main/kotlin/io/tolgee/batch/BatchJobConcurrentLauncher.kt (1)
266-269: LGTM! Good optimization with caching and early return.The caching of
maxPerJobConcurrencyavoids redundant service calls, and the early return for unlimited concurrency (-1) is a clear performance win.backend/data/src/main/kotlin/io/tolgee/batch/ProgressManager.kt (1)
44-46: Critical fix: Status now properly updated for existing execution states.This change addresses the core issue described in the PR. Previously, when an execution state already existed in the cache, the code would return true without updating its status to RUNNING. This mutation ensures the status is consistently set whether the execution state is new or pre-existing, which is essential for the concurrency control logic in
BatchJobConcurrentLauncherto work correctly.Minor note: The safe call operator
?.on line 45 is redundant since we already verifiedit[executionId] != nullon line 44, but it's harmless and adds defensive programming.backend/app/src/test/kotlin/io/tolgee/batch/BatchJobTestUtil.kt (1)
10-10: LGTM! Well-structured test helpers for concurrency validation.The new test helpers properly support validating the
maxPerJobConcurrencybehavior:
runMtJobfollows the established pattern of other job creation helpers in this file.assertAllowedMaxPerJobConcurrencyprovides a straightforward way to verify the configured concurrency limit.assertMaxPerJobConcurrencyIsLessThanOrEqualTovalidates actual runtime behavior by checking both the running jobs count and ensuring the queue is idle, which is a thorough approach.These additions align well with the PR's objective to ensure concurrency limits are respected.
Also applies to: 47-47, 452-493
bdshadow
force-pushed
the
fix/respect-maxPerJobConcurrency
branch
from
63616b2 to
6763e31
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
backend/app/src/test/kotlin/io/tolgee/api/v2/controllers/batch/BatchJobManagementControllerCancellationTest.kt(1 hunks)backend/app/src/test/kotlin/io/tolgee/batch/AbstractBatchJobsGeneralTest.kt(1 hunks)backend/app/src/test/kotlin/io/tolgee/batch/BatchJobTestUtil.kt(3 hunks)backend/data/src/main/kotlin/io/tolgee/batch/BatchJobConcurrentLauncher.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/batch/ProgressManager.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/batch/processors/MachineTranslationChunkProcessor.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/model/batch/BatchJob.kt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/app/src/test/kotlin/io/tolgee/batch/BatchJobTestUtil.kt
- backend/app/src/test/kotlin/io/tolgee/batch/AbstractBatchJobsGeneralTest.kt
🧰 Additional context used
📓 Path-based instructions (3)
backend/**/*.kt
📄 CodeRabbit inference engine (AGENTS.md)
backend/**/*.kt: After modifying JPA entities, run./gradlew diffChangeLogto generate Liquibase changelog entries (add--no-daemonflag if docker command not found)
Run./gradlew ktlintFormatbefore committing code
Files:
backend/app/src/test/kotlin/io/tolgee/api/v2/controllers/batch/BatchJobManagementControllerCancellationTest.ktbackend/data/src/main/kotlin/io/tolgee/model/batch/BatchJob.ktbackend/data/src/main/kotlin/io/tolgee/batch/processors/MachineTranslationChunkProcessor.ktbackend/data/src/main/kotlin/io/tolgee/batch/ProgressManager.ktbackend/data/src/main/kotlin/io/tolgee/batch/BatchJobConcurrentLauncher.kt
backend/**/*Test.kt
📄 CodeRabbit inference engine (AGENTS.md)
backend/**/*Test.kt: Use TestData classes for test setup with TestDataService for saving and cleaning test data in unit tests
Use.andAssertThatJsonfor testing API JSON responses in tests
Files:
backend/app/src/test/kotlin/io/tolgee/api/v2/controllers/batch/BatchJobManagementControllerCancellationTest.kt
**/*.{ts,tsx,js,jsx,kt,kts}
⚙️ CodeRabbit configuration file
As part of review, please check if the file follows 'The Stepdown Rule': The most important, high-level concepts should be at the top (like a newspaper headline and opening paragraph). Details should increase as you read downward. Functions should be ordered so that a caller appears before the functions it calls. When working with JavaScript components, we allow the main component to live at the bottom of the file as an exception to the rule.
Files:
backend/app/src/test/kotlin/io/tolgee/api/v2/controllers/batch/BatchJobManagementControllerCancellationTest.ktbackend/data/src/main/kotlin/io/tolgee/model/batch/BatchJob.ktbackend/data/src/main/kotlin/io/tolgee/batch/processors/MachineTranslationChunkProcessor.ktbackend/data/src/main/kotlin/io/tolgee/batch/ProgressManager.ktbackend/data/src/main/kotlin/io/tolgee/batch/BatchJobConcurrentLauncher.kt
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Frontend static check 🪲
- GitHub Check: Build backend 🏗️
- GitHub Check: Build frontend 🏗️
🔇 Additional comments (4)
backend/app/src/test/kotlin/io/tolgee/api/v2/controllers/batch/BatchJobManagementControllerCancellationTest.kt (1)
71-73: LGTM!The mock configuration appropriately simulates unlimited concurrency for testing multi-threaded cancellation behavior.
backend/data/src/main/kotlin/io/tolgee/model/batch/BatchJob.kt (1)
66-71: LGTM!The KDoc clearly documents the
maxPerJobConcurrencyproperty, explaining that -1 means unlimited concurrency and referencing the implementation for additional details.backend/data/src/main/kotlin/io/tolgee/batch/BatchJobConcurrentLauncher.kt (1)
264-277: LGTM!Caching
maxPerJobConcurrencyand adding an early return for unlimited concurrency (-1) improves both performance and correctness by ensuring the value remains consistent throughout the check.backend/data/src/main/kotlin/io/tolgee/batch/processors/MachineTranslationChunkProcessor.kt (1)
50-59: LGTM!The documentation clearly explains the rationale for limiting MT job concurrency to 1, citing rate limit concerns and credit calculation timing constraints.
bdshadow
force-pushed
the
fix/respect-maxPerJobConcurrency
branch
from
6763e31 to
6f6a869
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
backend/app/src/test/kotlin/io/tolgee/batch/AbstractBatchJobsGeneralTest.kt (1)
347-360: Consider deriving maxConcurrency from the processor.The test hardcodes
maxConcurrency = 1based on a comment assumption. For better maintainability, consider reading this value directly frommachineTranslationChunkProcessor.getMaxPerJobConcurrency()so the test automatically adapts if the processor's concurrency setting changes.🔎 Proposed refactor
@Test fun `mt job respects maxPerJobConcurrency`() { val mtJob = util.runMtJob(100) // by now mt jobs have maxPerJobConcurrency = 1 and this won't probably change, // because if parallelized we will start hitting rate limits on OpenAI and even get over it. // At the beginning, we only check that the organization has availableCredits > 0 and // only when we have the result from OpenAI, we are able to calculate how many credits to charge the organization. - val maxConcurrency = 1 + val maxConcurrency = machineTranslationChunkProcessor.getMaxPerJobConcurrency() util.assertAllowedMaxPerJobConcurrency(mtJob, maxConcurrency) util.assertMaxPerJobConcurrencyIsLessThanOrEqualTo(maxConcurrency) util.waitForJobSuccess(mtJob) util.assertJobUnlocked() }backend/app/src/test/kotlin/io/tolgee/batch/BatchJobTestUtil.kt (2)
47-47: Remove unnecessary import.The
kotlin.applyextension function is part of the Kotlin standard library and doesn't require an explicit import.🔎 Proposed fix
-import kotlin.apply import kotlin.coroutines.CoroutineContext
452-474: LGTM with optional simplification.The method follows the established pattern of other job creation helpers. Implementation is correct.
For consistency across the file, consider simplifying the keyIds generation:
Optional: Simplify keyIds generation
request = MachineTranslationRequest().apply { - keyIds = (1L..keyCount).map { it } + keyIds = (1L..keyCount).toList() targetLanguageIds =Note: This same simplification could be applied to other methods in the file (
runChunkedJobat lines 338, 442).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
backend/app/src/test/kotlin/io/tolgee/api/v2/controllers/batch/BatchJobManagementControllerCancellationTest.kt(1 hunks)backend/app/src/test/kotlin/io/tolgee/batch/AbstractBatchJobsGeneralTest.kt(1 hunks)backend/app/src/test/kotlin/io/tolgee/batch/BatchJobTestUtil.kt(3 hunks)backend/data/src/main/kotlin/io/tolgee/batch/BatchJobConcurrentLauncher.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/batch/ProgressManager.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/batch/processors/MachineTranslationChunkProcessor.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/model/batch/BatchJob.kt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- backend/data/src/main/kotlin/io/tolgee/batch/ProgressManager.kt
- backend/data/src/main/kotlin/io/tolgee/batch/BatchJobConcurrentLauncher.kt
- backend/data/src/main/kotlin/io/tolgee/batch/processors/MachineTranslationChunkProcessor.kt
🧰 Additional context used
📓 Path-based instructions (3)
backend/**/*.kt
📄 CodeRabbit inference engine (AGENTS.md)
backend/**/*.kt: After modifying JPA entities, run./gradlew diffChangeLogto generate Liquibase changelog entries (add--no-daemonflag if docker command not found)
Run./gradlew ktlintFormatbefore committing code
Files:
backend/app/src/test/kotlin/io/tolgee/batch/AbstractBatchJobsGeneralTest.ktbackend/app/src/test/kotlin/io/tolgee/batch/BatchJobTestUtil.ktbackend/app/src/test/kotlin/io/tolgee/api/v2/controllers/batch/BatchJobManagementControllerCancellationTest.ktbackend/data/src/main/kotlin/io/tolgee/model/batch/BatchJob.kt
backend/**/*Test.kt
📄 CodeRabbit inference engine (AGENTS.md)
backend/**/*Test.kt: Use TestData classes for test setup with TestDataService for saving and cleaning test data in unit tests
Use.andAssertThatJsonfor testing API JSON responses in tests
Files:
backend/app/src/test/kotlin/io/tolgee/batch/AbstractBatchJobsGeneralTest.ktbackend/app/src/test/kotlin/io/tolgee/api/v2/controllers/batch/BatchJobManagementControllerCancellationTest.kt
**/*.{ts,tsx,js,jsx,kt,kts}
⚙️ CodeRabbit configuration file
As part of review, please check if the file follows 'The Stepdown Rule': The most important, high-level concepts should be at the top (like a newspaper headline and opening paragraph). Details should increase as you read downward. Functions should be ordered so that a caller appears before the functions it calls. When working with JavaScript components, we allow the main component to live at the bottom of the file as an exception to the rule.
Files:
backend/app/src/test/kotlin/io/tolgee/batch/AbstractBatchJobsGeneralTest.ktbackend/app/src/test/kotlin/io/tolgee/batch/BatchJobTestUtil.ktbackend/app/src/test/kotlin/io/tolgee/api/v2/controllers/batch/BatchJobManagementControllerCancellationTest.ktbackend/data/src/main/kotlin/io/tolgee/model/batch/BatchJob.kt
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build frontend 🏗️
- GitHub Check: Frontend static check 🪲
- GitHub Check: Build backend 🏗️
🔇 Additional comments (2)
backend/app/src/test/kotlin/io/tolgee/api/v2/controllers/batch/BatchJobManagementControllerCancellationTest.kt (1)
71-73: LGTM!The stub correctly simulates unlimited concurrency (-1) to enable multi-threaded cancellation testing. The comment clearly explains the rationale.
backend/app/src/test/kotlin/io/tolgee/batch/BatchJobTestUtil.kt (1)
476-493: LGTM!The assertion helpers correctly verify both the configured limit and actual runtime behavior. The
assertMaxPerJobConcurrencyIsLessThanOrEqualTomethod uses polling to continuously monitor that concurrent execution respects the limit while processing the queue.
| /** | ||
| * The maximum number of coroutines among all tolgee instances (e.g. k8s pods). The default value is -1, | ||
| * which means that the concurrency is not limited. | ||
| * (check `BatchJobConcurrentLauncher#ExecutionQueueItem.trySetRunningState()` for more info). | ||
| */ |
There was a problem hiding this comment.
Adjust KDoc reference format for nested class method.
The KDoc reference format should use . for the nested class and # for the method: BatchJobConcurrentLauncher.ExecutionQueueItem#trySetRunningState() instead of BatchJobConcurrentLauncher#ExecutionQueueItem.trySetRunningState().
🔎 Proposed fix
/**
* The maximum number of coroutines among all tolgee instances (e.g. k8s pods). The default value is -1,
* which means that the concurrency is not limited.
- * (check `BatchJobConcurrentLauncher#ExecutionQueueItem.trySetRunningState()` for more info).
+ * (check `BatchJobConcurrentLauncher.ExecutionQueueItem#trySetRunningState()` for more info).
*/📝 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.
| /** | |
| * The maximum number of coroutines among all tolgee instances (e.g. k8s pods). The default value is -1, | |
| * which means that the concurrency is not limited. | |
| * (check `BatchJobConcurrentLauncher#ExecutionQueueItem.trySetRunningState()` for more info). | |
| */ | |
| /** | |
| * The maximum number of coroutines among all tolgee instances (e.g. k8s pods). The default value is -1, | |
| * which means that the concurrency is not limited. | |
| * (check `BatchJobConcurrentLauncher.ExecutionQueueItem#trySetRunningState()` for more info). | |
| */ |
🤖 Prompt for AI Agents
In backend/data/src/main/kotlin/io/tolgee/model/batch/BatchJob.kt around lines
66 to 70, the KDoc reference for the nested class method is formatted
incorrectly; update the reference from
`BatchJobConcurrentLauncher#ExecutionQueueItem.trySetRunningState()` to use the
correct KotlinDoc style for a nested class and method:
`BatchJobConcurrentLauncher.ExecutionQueueItem#trySetRunningState()`. Ensure
only the reference text in the comment is changed and no other content is
altered.
6f6a869 to
6eb3e95
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
backend/data/src/main/kotlin/io/tolgee/configuration/tolgee/BatchProperties.kt (1)
15-24: Fix description formatting for better documentation generation.The description contains formatting issues that will produce poorly-rendered documentation:
"\n."on line 17 creates an orphaned period on a new line- Mid-sentence line breaks make the text harder to read
- The phrase "we only check, that" has an unnecessary comma
🔎 Suggested formatting improvement
@DocProperty( description = - "Concurrency among all tolgee instances per one machine translation job\n." + - "Higher concurrency provides faster and distributed processing, but can lead to hitting rate limit\n" + - "on OpenAI, as well as getting over the limit of availableCredits: at the beginning, we only check, that\n" + - "the organization has `availableCredits > 0` and only when we have the result from OpenAI,we are able to\n" + - "calculate how many credits to charge the organization. In the Tolgee Cloud it is set to 1. ", + "Concurrency among all Tolgee instances per one machine translation job. " + + "Higher concurrency provides faster and distributed processing, but can lead to hitting rate limits " + + "on OpenAI, as well as exceeding the availableCredits limit: at the beginning, we only check that " + + "the organization has `availableCredits > 0`, and only when we have the result from OpenAI, we are able to " + + "calculate how many credits to charge the organization. In the Tolgee Cloud it is set to 1.", defaultValue = "-1", defaultExplanation = "Unlimited (within tolgee.batch.concurrency)", )backend/data/src/main/kotlin/io/tolgee/batch/ProgressManager.kt (1)
44-51: Refactor for clearer null handling with smart casting.The terminal state check logic is correct, but the null handling could be improved. Currently, line 44 checks
it[executionId] != null, but Kotlin doesn't smart-cast map access, socurrentStateon line 45 remains nullable (ExecutionState?). The safe call operator on line 50 means ifcurrentStatewere somehow null, it would be a no-op, yet line 51 returnstruesuggesting successful update—a logical inconsistency.While unlikely to cause issues in practice due to locking, refactoring with explicit null checking makes the intent clearer and leverages Kotlin's smart casting.
🔎 Proposed refactoring using smart casting
- if (canRunFn(it)) { - if (it[executionId] != null) { - val currentState = it[executionId] - // Don't overwrite terminal states - if (currentState?.status?.completed == true) { - return@updateState false - } - currentState?.status = BatchJobChunkExecutionStatus.RUNNING - return@updateState true - } + if (canRunFn(it)) { + val currentState = it[executionId] + if (currentState != null) { + // Don't overwrite terminal states + if (currentState.status.completed) { + return@updateState false + } + currentState.status = BatchJobChunkExecutionStatus.RUNNING + return@updateState true + } it[executionId] = ExecutionState( successTargets = listOf(),This approach:
- Uses explicit null check for smart casting
- Eliminates safe call operators (clearer that currentState is non-null after check)
- More idiomatic Kotlin
Reminder: Run ktlintFormat before committing.
As per coding guidelines, please run
./gradlew ktlintFormatbefore committing to ensure code style consistency.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
backend/app/src/test/kotlin/io/tolgee/batch/AbstractBatchJobsGeneralTest.kt(3 hunks)backend/app/src/test/kotlin/io/tolgee/batch/BatchJobTestUtil.kt(3 hunks)backend/data/src/main/kotlin/io/tolgee/batch/BatchJobConcurrentLauncher.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/batch/ProgressManager.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/batch/processors/MachineTranslationChunkProcessor.kt(3 hunks)backend/data/src/main/kotlin/io/tolgee/configuration/tolgee/BatchProperties.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/model/batch/BatchJob.kt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/data/src/main/kotlin/io/tolgee/batch/BatchJobConcurrentLauncher.kt
- backend/data/src/main/kotlin/io/tolgee/model/batch/BatchJob.kt
🧰 Additional context used
📓 Path-based instructions (3)
backend/**/*.kt
📄 CodeRabbit inference engine (AGENTS.md)
backend/**/*.kt: After modifying JPA entities, run./gradlew diffChangeLogto generate Liquibase changelog entries (add--no-daemonflag if docker command not found)
Run./gradlew ktlintFormatbefore committing code
Files:
backend/data/src/main/kotlin/io/tolgee/configuration/tolgee/BatchProperties.ktbackend/app/src/test/kotlin/io/tolgee/batch/AbstractBatchJobsGeneralTest.ktbackend/data/src/main/kotlin/io/tolgee/batch/processors/MachineTranslationChunkProcessor.ktbackend/data/src/main/kotlin/io/tolgee/batch/ProgressManager.ktbackend/app/src/test/kotlin/io/tolgee/batch/BatchJobTestUtil.kt
**/*.{ts,tsx,js,jsx,kt,kts}
⚙️ CodeRabbit configuration file
As part of review, please check if the file follows 'The Stepdown Rule': The most important, high-level concepts should be at the top (like a newspaper headline and opening paragraph). Details should increase as you read downward. Functions should be ordered so that a caller appears before the functions it calls. When working with JavaScript components, we allow the main component to live at the bottom of the file as an exception to the rule.
Files:
backend/data/src/main/kotlin/io/tolgee/configuration/tolgee/BatchProperties.ktbackend/app/src/test/kotlin/io/tolgee/batch/AbstractBatchJobsGeneralTest.ktbackend/data/src/main/kotlin/io/tolgee/batch/processors/MachineTranslationChunkProcessor.ktbackend/data/src/main/kotlin/io/tolgee/batch/ProgressManager.ktbackend/app/src/test/kotlin/io/tolgee/batch/BatchJobTestUtil.kt
backend/**/*Test.kt
📄 CodeRabbit inference engine (AGENTS.md)
backend/**/*Test.kt: Use TestData classes for test setup with TestDataService for saving and cleaning test data in unit tests
Use.andAssertThatJsonfor testing API JSON responses in tests
Files:
backend/app/src/test/kotlin/io/tolgee/batch/AbstractBatchJobsGeneralTest.kt
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Frontend static check 🪲
- GitHub Check: Build backend 🏗️
- GitHub Check: Build frontend 🏗️
🔇 Additional comments (5)
backend/data/src/main/kotlin/io/tolgee/batch/processors/MachineTranslationChunkProcessor.kt (1)
8-8: LGTM! Clean refactoring to use configuration.The change properly replaces the hardcoded concurrency value with the configurable
maxPerMtJobConcurrencyproperty fromBatchProperties. The dependency injection is clean, and the implementation correctly delegates to the configuration.Also applies to: 20-20, 51-53
backend/app/src/test/kotlin/io/tolgee/batch/BatchJobTestUtil.kt (1)
452-493: LGTM! Well-structured test helpers for MT job concurrency validation.The three new helper methods provide clear test support for the MT job concurrency feature:
runMtJob()creates a properly configured machine translation batch jobassertAllowedMaxPerJobConcurrency()validates the job's concurrency configurationassertMaxPerJobConcurrencyIsLessThanOrEqualTo()verifies runtime concurrency constraints with appropriate pollingThe implementation correctly uses
waitForto handle async execution and checks both running jobs and queue state.backend/app/src/test/kotlin/io/tolgee/batch/AbstractBatchJobsGeneralTest.kt (2)
33-34: LGTM! Proper injection for configuration testing.The autowired
BatchPropertiesfield enables the new test to verify the MT job concurrency behavior.
351-366: LGTM! Comprehensive test with proper cleanup.The test correctly validates that MT jobs respect the
maxPerMtJobConcurrencysetting. The try-finally block ensures the configuration is restored even if assertions fail, preventing test pollution. The test flow is logical:
- Override configuration
- Run MT job
- Assert concurrency limits are enforced
- Verify successful completion
- Restore original configuration
Note that this test mutates shared state (
batchProperties), but the cleanup in the finally block and the@WebsocketTestisolation should prevent interference with other tests.backend/data/src/main/kotlin/io/tolgee/batch/ProgressManager.kt (1)
37-65: Terminal state protection successfully addresses the previous concern.The addition of the terminal state check prevents executions in completed states (CANCELLED, FAILED, SUCCESS) from being transitioned back to RUNNING, which was the critical issue flagged in previous reviews. The overall flow is sound:
- Concurrency limits checked via
canRunFn- Terminal states prevented from being overwritten
- New executions created when needed
… chunks was not updated in redis/local queue Signed-off-by: dmitrii.bocharov <[email protected]>
bdshadow
force-pushed
the
fix/respect-maxPerJobConcurrency
branch
from
6eb3e95 to
1a26b29
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
backend/data/src/main/kotlin/io/tolgee/batch/processors/MachineTranslationChunkProcessor.kt (1)
12-12: Remove unused import.The
ProjectServiceimport is no longer used after replacing the constructor parameter withBatchProperties.🔎 Proposed fix
-import io.tolgee.service.project.ProjectService
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
backend/app/src/test/kotlin/io/tolgee/batch/AbstractBatchJobsGeneralTest.ktbackend/app/src/test/kotlin/io/tolgee/batch/BatchJobTestUtil.ktbackend/data/src/main/kotlin/io/tolgee/batch/BatchJobConcurrentLauncher.ktbackend/data/src/main/kotlin/io/tolgee/batch/ProgressManager.ktbackend/data/src/main/kotlin/io/tolgee/batch/processors/MachineTranslationChunkProcessor.ktbackend/data/src/main/kotlin/io/tolgee/configuration/tolgee/BatchProperties.ktbackend/data/src/main/kotlin/io/tolgee/model/batch/BatchJob.kt
🚧 Files skipped from review as they are similar to previous changes (5)
- backend/data/src/main/kotlin/io/tolgee/batch/BatchJobConcurrentLauncher.kt
- backend/data/src/main/kotlin/io/tolgee/model/batch/BatchJob.kt
- backend/data/src/main/kotlin/io/tolgee/batch/ProgressManager.kt
- backend/data/src/main/kotlin/io/tolgee/configuration/tolgee/BatchProperties.kt
- backend/app/src/test/kotlin/io/tolgee/batch/AbstractBatchJobsGeneralTest.kt
🧰 Additional context used
📓 Path-based instructions (2)
backend/**/*.kt
📄 CodeRabbit inference engine (AGENTS.md)
backend/**/*.kt: After modifying JPA entities, run./gradlew diffChangeLogto generate Liquibase changelog entries (add--no-daemonflag if docker command not found)
Run./gradlew ktlintFormatbefore committing code
Files:
backend/data/src/main/kotlin/io/tolgee/batch/processors/MachineTranslationChunkProcessor.ktbackend/app/src/test/kotlin/io/tolgee/batch/BatchJobTestUtil.kt
**/*.{ts,tsx,js,jsx,kt,kts}
⚙️ CodeRabbit configuration file
As part of review, please check if the file follows 'The Stepdown Rule': The most important, high-level concepts should be at the top (like a newspaper headline and opening paragraph). Details should increase as you read downward. Functions should be ordered so that a caller appears before the functions it calls. When working with JavaScript components, we allow the main component to live at the bottom of the file as an exception to the rule.
Files:
backend/data/src/main/kotlin/io/tolgee/batch/processors/MachineTranslationChunkProcessor.ktbackend/app/src/test/kotlin/io/tolgee/batch/BatchJobTestUtil.kt
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build backend 🏗️
- GitHub Check: Build frontend 🏗️
- GitHub Check: Frontend static check 🪲
🔇 Additional comments (5)
backend/app/src/test/kotlin/io/tolgee/batch/BatchJobTestUtil.kt (4)
10-10: LGTM!Import is needed for the new
runMtJobhelper function.
476-484: LGTM!Clean assertion helper that follows the pattern of other assertion functions in this utility class.
486-493: LGTM!This assertion helper correctly validates that the maxPerJobConcurrency limit is respected by checking both the running jobs count and ensuring the queue is empty. The implementation aligns well with the PR objective of fixing the maxPerJobConcurrency issue.
452-474: The function is correct. The test data properly initializes the Czech language viaprojectBuilder.addCzech()on line 12 ofBatchJobsTestData.kt, making the null-assertion operator on line 465 safe.backend/data/src/main/kotlin/io/tolgee/batch/processors/MachineTranslationChunkProcessor.kt (1)
20-20: This is a newly created processor, not a modification of existing behavior.MachineTranslationChunkProcessor.kt is a new file introduced in this PR, not a refactoring of existing code. The implementation correctly uses the configurable
batchProperties.maxPerMtJobConcurrencyproperty, which defaults to -1 (unlimited concurrency) as documented. This is the intended design for the new processor, not a behavioral change from a previous hardcoded value of 1.If you have concerns about the default unlimited concurrency behavior for machine translation jobs, please clarify whether that's by design and add documentation if needed—but this isn't a regression from prior behavior.
Likely an incorrect or invalid review comment.
## [3.145.3](v3.145.2...v3.145.3) (2026-01-05) ### Bug Fixes * update status of pending chunks in redis/local queue ([#3355](#3355)) ([959fda0](959fda0))
2 participants
… chunks was not updated in redis/local queue
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.