[NAE-2250] Optimize Elasticsearch reindexing and bulk operations for performance and reliability #378
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Updated parent POM version from `7.0.0-RC8.1` to `7.0.0-RC9` across all module POM files.
…performance and reliability Replaced direct Elasticsearch operations with ElasticQueueManager for better batch processing and asynchronous handling of indexing and deletion tasks. Updated configuration to include queue properties and standardized dependency handling with constructor injection. This improves performance, scalability, and maintainability of Elasticsearch operations.
WalkthroughRewires Elasticsearch DI to constructor injection; adds ElasticQueueManager for queued bulk index/delete via ElasticsearchClient; case index/remove now enqueue BulkOperationWrapper items; introduces queue properties; disables automatic index creation on adapter documents; ElasticTask made Serializable; bulk docs mapped via ElasticsearchConverter. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant CaseService as ElasticCaseService
participant IndexQueue as ElasticQueueManager (Index)
participant DeleteQueue as ElasticQueueManager (Delete)
participant Scheduler as ScheduledExecutor
participant ES as Elasticsearch
Note over Client,ES `#e8f3ff`: Queue-based bulk indexing & deletion flow
Client->>CaseService: index(case)
CaseService->>IndexQueue: push(BulkOperationWrapper{index op, event})
IndexQueue->>IndexQueue: enqueue / checkQueue()
IndexQueue->>Scheduler: resetTimer()
Scheduler-->>IndexQueue: timer fires or batch threshold reached
IndexQueue->>ES: bulk(request, refresh: configured)
ES-->>IndexQueue: success / failure
alt failure
IndexQueue->>IndexQueue: log / reschedule
end
Client->>CaseService: remove(caseId)
CaseService->>DeleteQueue: push(BulkOperationWrapper{delete op, event})
DeleteQueue->>Scheduler: resetTimer()
Scheduler-->>DeleteQueue: timer fires or batch threshold reached
DeleteQueue->>ES: bulk(delete request, refresh: configured)
ES-->>DeleteQueue: success / failure
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticCaseService.java (1)
118-120: Breaking change:indexNow()no longer provides immediate indexing.The
indexNow()method name implies synchronous, immediate indexing, but it now simply delegates toindex(), which only queues the operation. This is a breaking semantic change.Impact:
- Callers expecting immediate Elasticsearch visibility will be broken
- Code paths requiring immediate indexing for correctness (e.g., before a search) will fail
- The method's contract is no longer honored
Consider one of these solutions:
Option 1 (Recommended): Implement true immediate indexing:
@Override public void indexNow(ElasticCase useCase) { - index(useCase); + // Perform immediate, non-queued indexing + Optional<com.netgrif.application.engine.adapter.spring.elastic.domain.ElasticCase> elasticCaseOptional = repository.findById(useCase.getId()); + if (elasticCaseOptional.isEmpty()) { + repository.save((com.netgrif.application.engine.adapter.spring.elastic.domain.ElasticCase) useCase); + } else { + com.netgrif.application.engine.adapter.spring.elastic.domain.ElasticCase elasticCase = elasticCaseOptional.get(); + elasticCase.update(useCase); + repository.save(elasticCase); + } + log.debug("[" + useCase.getId() + "]: Case \"" + useCase.getTitle() + "\" indexed immediately"); + publisher.publishEvent(new IndexCaseEvent(useCase)); }Option 2: Flush the queue after enqueuing:
@Override public void indexNow(ElasticCase useCase) { index(useCase); + caseElasticIndexQueueManager.flush(); }Option 3: Deprecate and rename if immediate indexing is no longer needed:
+@Deprecated @Override public void indexNow(ElasticCase useCase) { + // Deprecated: This method now queues like index(). Use index() directly. index(useCase); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
application-engine/src/main/java/com/netgrif/application/engine/configuration/ElasticServiceConfiguration.java(3 hunks)application-engine/src/main/java/com/netgrif/application/engine/configuration/properties/DataConfigurationProperties.java(2 hunks)application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticCaseService.java(2 hunks)application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticQueueManager.java(1 hunks)nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/ElasticCase.java(1 hunks)nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/ElasticPetriNet.java(1 hunks)nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/ElasticTask.java(1 hunks)
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: machacjozef
Repo: netgrif/application-engine PR: 367
File: application-engine/src/main/resources/application.yaml:24-24
Timestamp: 2025-10-20T11:44:44.907Z
Learning: In the netgrif/application-engine project, the correction of the Elasticsearch task index name from "_taks" to "_task" in application.yaml was approved by maintainer machacjozef, indicating that any data migration concerns for this typo fix are handled separately or not applicable to their deployment scenario.
Learnt from: tuplle
Repo: netgrif/application-engine PR: 331
File: application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticPetriNetService.java:45-46
Timestamp: 2025-07-29T17:19:18.300Z
Learning: In ElasticPetriNetService class, petriNetService is properly initialized using Lazy setter injection rather than constructor injection. This pattern with Lazy Autowired setter methods is commonly used in Spring to resolve circular dependencies and is a valid alternative to constructor injection.
📚 Learning: 2025-10-20T11:44:44.907Z
Learnt from: machacjozef
Repo: netgrif/application-engine PR: 367
File: application-engine/src/main/resources/application.yaml:24-24
Timestamp: 2025-10-20T11:44:44.907Z
Learning: In the netgrif/application-engine project, the correction of the Elasticsearch task index name from "_taks" to "_task" in application.yaml was approved by maintainer machacjozef, indicating that any data migration concerns for this typo fix are handled separately or not applicable to their deployment scenario.
Applied to files:
nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/ElasticTask.javaapplication-engine/src/main/java/com/netgrif/application/engine/configuration/ElasticServiceConfiguration.javaapplication-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticCaseService.java
📚 Learning: 2025-08-19T20:07:15.621Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 339
File: application-engine/src/test/groovy/com/netgrif/application/engine/filters/FilterImportExportTest.groovy:341-341
Timestamp: 2025-08-19T20:07:15.621Z
Learning: In the Elasticsearch migration, the stringId property was removed from ElasticCase and ElasticTask, with the id property now containing the MongoDB case/task object ID directly. This consolidation eliminates redundancy and simplifies the data model.
Applied to files:
nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/ElasticTask.javanae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/ElasticCase.javaapplication-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticCaseService.java
📚 Learning: 2025-08-19T20:13:40.087Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 339
File: nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/CaseField.java:16-16
Timestamp: 2025-08-19T20:13:40.087Z
Learning: In CaseField.java, fulltextValue is mapped as a keyword field type in Elasticsearch (for exact matches, filtering, aggregations), while the separate caseValue field serves different Elasticsearch query requirements, allowing the system to support multiple query patterns on the same data through different field mappings.
Applied to files:
nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/ElasticCase.javaapplication-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticCaseService.java
📚 Learning: 2025-08-19T20:07:43.748Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 339
File: nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/CaseField.java:16-16
Timestamp: 2025-08-19T20:07:43.748Z
Learning: In CaseField.java, the separate caseValue field (List<String>) is intentionally maintained alongside fulltextValue for specific Elasticsearch query requirements, rather than being derived on-the-fly from fulltextValue.
Applied to files:
nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/ElasticCase.java
📚 Learning: 2025-07-29T17:19:18.300Z
Learnt from: tuplle
Repo: netgrif/application-engine PR: 331
File: application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticPetriNetService.java:45-46
Timestamp: 2025-07-29T17:19:18.300Z
Learning: In ElasticPetriNetService class, petriNetService is properly initialized using Lazy setter injection rather than constructor injection. This pattern with Lazy Autowired setter methods is commonly used in Spring to resolve circular dependencies and is a valid alternative to constructor injection.
Applied to files:
nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/ElasticPetriNet.javaapplication-engine/src/main/java/com/netgrif/application/engine/configuration/ElasticServiceConfiguration.javaapplication-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticCaseService.java
🧬 Code graph analysis (1)
nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/ElasticCase.java (1)
nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/workflow/domain/Task.java (1)
Document(23-59)
⏰ 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). (4)
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: Build
🔇 Additional comments (6)
application-engine/src/main/java/com/netgrif/application/engine/configuration/ElasticServiceConfiguration.java (4)
9-24: Excellent refactoring to constructor-based injection!The migration from field-based
@Autowiredto constructor-based injection using Lombok's@RequiredArgsConstructoris a best practice that improves testability and makes dependencies explicit.
32-39: LGTM!All dependencies are correctly declared as final fields for constructor injection. The new dependencies align with the updated
ElasticCaseServiceconstructor signature.
60-69: LGTM!The bean creation properly wires all dependencies into the
ElasticCaseServiceconstructor, including the new queue-based operation dependencies.
80-89: LGTM!The reindexing bean creation properly wires all dependencies, correctly using
reindexingTaskCaseExecutor()for the specialized executor.application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticCaseService.java (2)
61-62: LGTM!Separate queue managers for index and delete operations provide good separation of concerns for batch processing.
64-82: Queue lifecycle is properly managed—verification complete.The queue managers are correctly integrated with Spring's lifecycle:
- Queued operations execute on a configurable schedule via
scheduler.schedule()(called on eachpush())- On application shutdown,
@PreDestroy shutdown()explicitly flushes remaining items before terminating- Batch processing errors are handled by Spring's
@PreDestroyexception handling and logged with UUID tracking for debuggingBoth queue managers in the constructor will have
shutdown()invoked automatically by Spring.
...-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticCaseService.java
Outdated
Show resolved
Hide resolved
...-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticCaseService.java
Outdated
Show resolved
Hide resolved
...-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticCaseService.java
Outdated
Show resolved
Hide resolved
...engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticQueueManager.java
Show resolved
Hide resolved
...engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticQueueManager.java
Outdated
Show resolved
Hide resolved
.../src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/ElasticCase.java
Show resolved
Hide resolved
Retoocs
requested changes
Nov 4, 2025
...-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticCaseService.java
Outdated
Show resolved
Hide resolved
renczesstefan
commented
Nov 4, 2025
...engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticQueueManager.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticCaseService.java(2 hunks)
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: machacjozef
Repo: netgrif/application-engine PR: 367
File: application-engine/src/main/resources/application.yaml:24-24
Timestamp: 2025-10-20T11:44:44.907Z
Learning: In the netgrif/application-engine project, the correction of the Elasticsearch task index name from "_taks" to "_task" in application.yaml was approved by maintainer machacjozef, indicating that any data migration concerns for this typo fix are handled separately or not applicable to their deployment scenario.
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 339
File: application-engine/src/test/groovy/com/netgrif/application/engine/filters/FilterImportExportTest.groovy:341-341
Timestamp: 2025-08-19T20:07:15.621Z
Learning: In the Elasticsearch migration, the stringId property was removed from ElasticCase and ElasticTask, with the id property now containing the MongoDB case/task object ID directly. This consolidation eliminates redundancy and simplifies the data model.
📚 Learning: 2025-07-29T17:19:18.300Z
Learnt from: tuplle
Repo: netgrif/application-engine PR: 331
File: application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticPetriNetService.java:45-46
Timestamp: 2025-07-29T17:19:18.300Z
Learning: In ElasticPetriNetService class, petriNetService is properly initialized using Lazy setter injection rather than constructor injection. This pattern with Lazy Autowired setter methods is commonly used in Spring to resolve circular dependencies and is a valid alternative to constructor injection.
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticCaseService.java
📚 Learning: 2025-08-19T20:07:15.621Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 339
File: application-engine/src/test/groovy/com/netgrif/application/engine/filters/FilterImportExportTest.groovy:341-341
Timestamp: 2025-08-19T20:07:15.621Z
Learning: In the Elasticsearch migration, the stringId property was removed from ElasticCase and ElasticTask, with the id property now containing the MongoDB case/task object ID directly. This consolidation eliminates redundancy and simplifies the data model.
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticCaseService.java
📚 Learning: 2025-10-20T11:44:44.907Z
Learnt from: machacjozef
Repo: netgrif/application-engine PR: 367
File: application-engine/src/main/resources/application.yaml:24-24
Timestamp: 2025-10-20T11:44:44.907Z
Learning: In the netgrif/application-engine project, the correction of the Elasticsearch task index name from "_taks" to "_task" in application.yaml was approved by maintainer machacjozef, indicating that any data migration concerns for this typo fix are handled separately or not applicable to their deployment scenario.
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticCaseService.java
📚 Learning: 2025-08-20T07:27:02.660Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 339
File: application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticTaskQueueManager.java:38-45
Timestamp: 2025-08-20T07:27:02.660Z
Learning: When reviewing ElasticTaskQueueManager changes, task.getTask().getId() returns the document identifier while task.getTaskId() returns the business task identifier. The queue operations should use consistent identifiers throughout the lifecycle (scheduling, processing, cleanup).
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticCaseService.java
📚 Learning: 2025-08-19T20:13:40.087Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 339
File: nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/CaseField.java:16-16
Timestamp: 2025-08-19T20:13:40.087Z
Learning: In CaseField.java, fulltextValue is mapped as a keyword field type in Elasticsearch (for exact matches, filtering, aggregations), while the separate caseValue field serves different Elasticsearch query requirements, allowing the system to support multiple query patterns on the same data through different field mappings.
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticCaseService.java
⏰ 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). (5)
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: Build
🔇 Additional comments (3)
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticCaseService.java (3)
64-82: Constructor injection pattern looks good.The explicit constructor with @lazy annotations for circular dependency prevention follows established patterns in this codebase. Queue managers are properly initialized with the correct index configuration.
Based on learnings.
85-94: LGTM! Delete operations correctly queued.Both delete methods properly enqueue deletion requests and accurately log the queued state. The log messages correctly indicate operations are queued rather than completed.
106-106: Use logger's formatter instead of string concatenation.The log statement uses string concatenation, which is inefficient and contradicts previous feedback. Logger formatters prevent unnecessary string operations when the log level is disabled.
Apply this diff:
- log.debug("[{}]: Case \"{}\" indexed", useCase.getId(), useCase.getTitle()); + log.debug("[{}]: Case \"{}\" queued for indexing", useCase.getId(), useCase.getTitle());Note: The formatter (
{}placeholders) is already correct; this comment primarily addresses the updated log message from the previous comment. However, verify that all other log statements in the method use parameterized logging consistently.Likely an incorrect or invalid review comment.
...-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticCaseService.java
Show resolved
Hide resolved
...-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticCaseService.java
Outdated
Show resolved
Hide resolved
Add a return statement for empty batches to prevent unnecessary processing. Implement error handling to requeue failed batches and log the exception details. Prevent scheduling new tasks when the scheduler is shut down.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticQueueManager.java (1)
117-127: Critical: Timer cancellation prevents flushing under steady traffic + race condition.This method has two critical issues:
Issue 1: Steady traffic prevents flushing
Your earlier response suggested
cancel(false)allows the task to finish, but this doesn't address the core scenario.cancel(false)means:
- If the task is already running, don't interrupt it (let it finish)
- If the task hasn't started yet, cancel it (prevent it from running)
Under steady traffic where items arrive faster than the configured delay, each
push()call cancels the pending-but-not-yet-started task and schedules a new one. The flush never executes because the timer keeps resetting before the delay elapses.Example: With a 150ms delay, if items arrive every 100ms:
- T=0ms: push() schedules flush at T=150ms
- T=100ms: push() cancels pending flush, schedules new flush at T=250ms
- T=200ms: push() cancels pending flush, schedules new flush at T=350ms
- Result: flush never runs; queue grows unbounded → OOM
Issue 2: Race condition creates multiple untracked tasks
The pattern
getAndSet(null)→schedule()→set(newTask)is not atomic. Two concurrent threads can each schedule a task, but only the lastset()is tracked. The first scheduled task still executes but cannot be cancelled, leading to multiple concurrent flushes.Apply this diff to fix both issues by only scheduling when no task is pending:
private void resetTimer() { - ScheduledFuture<?> delayer = atomicDelayer.getAndSet(null); - if (delayer != null) { - delayer.cancel(false); - } if (scheduler.isShutdown()) { return; } - ScheduledFuture<?> newTask = scheduler.schedule(this::flush, queueProperties.getDelay(), queueProperties.getDelayUnit()); - atomicDelayer.set(newTask); + atomicDelayer.updateAndGet(existing -> { + // Only schedule a new task if no task is pending or the existing task is done + if (existing != null && !existing.isDone() && !existing.isCancelled()) { + return existing; // Keep the existing pending task + } + // Schedule a new flush task + return scheduler.schedule(this::flush, queueProperties.getDelay(), queueProperties.getDelayUnit()); + }); }This ensures:
- Under steady traffic, the first scheduled flush runs (queue gets flushed periodically)
- No race condition (atomic update)
- Only one flush task is scheduled at a time
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticQueueManager.java(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: machacjozef
Repo: netgrif/application-engine PR: 367
File: application-engine/src/main/resources/application.yaml:24-24
Timestamp: 2025-10-20T11:44:44.907Z
Learning: In the netgrif/application-engine project, the correction of the Elasticsearch task index name from "_taks" to "_task" in application.yaml was approved by maintainer machacjozef, indicating that any data migration concerns for this typo fix are handled separately or not applicable to their deployment scenario.
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 339
File: application-engine/src/test/groovy/com/netgrif/application/engine/filters/FilterImportExportTest.groovy:341-341
Timestamp: 2025-08-19T20:07:15.621Z
Learning: In the Elasticsearch migration, the stringId property was removed from ElasticCase and ElasticTask, with the id property now containing the MongoDB case/task object ID directly. This consolidation eliminates redundancy and simplifies the data model.
Learnt from: tuplle
Repo: netgrif/application-engine PR: 331
File: application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticPetriNetService.java:45-46
Timestamp: 2025-07-29T17:19:18.300Z
Learning: In ElasticPetriNetService class, petriNetService is properly initialized using Lazy setter injection rather than constructor injection. This pattern with Lazy Autowired setter methods is commonly used in Spring to resolve circular dependencies and is a valid alternative to constructor injection.
📚 Learning: 2025-11-04T11:47:03.883Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 378
File: nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/ElasticCase.java:16-16
Timestamp: 2025-11-04T11:47:03.883Z
Learning: In the netgrif/application-engine project, ElasticsearchRunner is responsible for creating Elasticsearch indexes automatically based on configuration. Spring Data's automatic index creation via the Document annotation's createIndex attribute should be disabled (set to false) to prevent conflicts with this centralized index management approach.
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticQueueManager.java
📚 Learning: 2025-10-20T11:44:44.907Z
Learnt from: machacjozef
Repo: netgrif/application-engine PR: 367
File: application-engine/src/main/resources/application.yaml:24-24
Timestamp: 2025-10-20T11:44:44.907Z
Learning: In the netgrif/application-engine project, the correction of the Elasticsearch task index name from "_taks" to "_task" in application.yaml was approved by maintainer machacjozef, indicating that any data migration concerns for this typo fix are handled separately or not applicable to their deployment scenario.
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticQueueManager.java
⏰ 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). (7)
- GitHub Check: Test
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
...engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticQueueManager.java
Show resolved
Hide resolved
Previously, the log message implied immediate indexing, which was misleading. The updated message correctly reflects that the case is added to the indexing queue. This improves clarity and aligns with the actual behavior of the method.
Added cancellation of the atomicDelayer to ensure proper task cleanup during shutdown. Implemented a timeout-based termination for the scheduler, with fallback to forced shutdown to handle interruptions and prevent potential resource leaks.
Replaced complex termination logic with a single `scheduler.shutdown()` call. This improves code readability and maintains the same functionality while reducing potential error points in the shutdown process.
Revised the shutdown method to ensure proper handling of queued tasks and prevent potential delays. Added cancellation of the atomic delayer and improved scheduler termination logic with a timeout and fallback to immediate shutdown. This enhances system stability and resource cleanup reliability.
Lowered the default scheduledExecutorPoolSize from 50 to 10. This change optimizes resource utilization and prevents over-allocation of threads, improving system performance under typical workloads.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticQueueManager.java (2)
129-139: Timer cancellation prevents flushing under steady load.This issue was previously flagged but remains unaddressed. The current implementation cancels any pending flush task and schedules a new one on every
push(). Under steady load where items arrive faster than the configured delay (e.g., items every 50ms with a 150ms delay), the scheduled flush is continuously cancelled and rescheduled, preventing any flush from ever executing. This causes unbounded queue growth and prevents indexing.The
cancel(false)parameter only prevents interruption of already executing tasks. For tasks that are scheduled but not yet running (the common case),cancel(false)will prevent them from executing.Consider implementing a "keep first scheduled flush" pattern:
private void resetTimer() { - ScheduledFuture<?> delayer = atomicDelayer.getAndSet(null); - if (delayer != null) { - delayer.cancel(false); - } if (scheduler.isShutdown()) { return; } - ScheduledFuture<?> newTask = scheduler.schedule(this::flush, queueProperties.getDelay(), queueProperties.getDelayUnit()); - atomicDelayer.set(newTask); + atomicDelayer.updateAndGet(existing -> { + if (existing != null && !existing.isDone() && !existing.isCancelled()) { + return existing; // Keep existing scheduled flush + } + return scheduler.schedule(this::flush, queueProperties.getDelay(), queueProperties.getDelayUnit()); + }); }This ensures that once a flush is scheduled, it will execute even if more items arrive, guaranteeing periodic flushing under continuous load.
66-81: Missingscheduler.shutdown()call prevents executor termination.The
awaitTermination()method at line 75 requires the executor to be shut down first. Without callingscheduler.shutdown()beforeawaitTermination(), the method will always wait for the full 30-second timeout and returnfalse, and the scheduler threads will never properly terminate.Apply this diff:
ScheduledFuture<?> delayer = atomicDelayer.getAndSet(null); if (delayer != null) { delayer.cancel(false); } + scheduler.shutdown(); while (!queue.isEmpty()) { flush(); } try { if (!scheduler.awaitTermination(30, TimeUnit.SECONDS)) { scheduler.shutdownNow(); } } catch (InterruptedException e) { scheduler.shutdownNow(); + Thread.currentThread().interrupt(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
application-engine/src/main/java/com/netgrif/application/engine/configuration/properties/DataConfigurationProperties.java(2 hunks)application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticQueueManager.java(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: machacjozef
Repo: netgrif/application-engine PR: 367
File: application-engine/src/main/resources/application.yaml:24-24
Timestamp: 2025-10-20T11:44:44.907Z
Learning: In the netgrif/application-engine project, the correction of the Elasticsearch task index name from "_taks" to "_task" in application.yaml was approved by maintainer machacjozef, indicating that any data migration concerns for this typo fix are handled separately or not applicable to their deployment scenario.
Learnt from: tuplle
Repo: netgrif/application-engine PR: 331
File: application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticPetriNetService.java:45-46
Timestamp: 2025-07-29T17:19:18.300Z
Learning: In ElasticPetriNetService class, petriNetService is properly initialized using Lazy setter injection rather than constructor injection. This pattern with Lazy Autowired setter methods is commonly used in Spring to resolve circular dependencies and is a valid alternative to constructor injection.
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 339
File: application-engine/src/test/groovy/com/netgrif/application/engine/filters/FilterImportExportTest.groovy:341-341
Timestamp: 2025-08-19T20:07:15.621Z
Learning: In the Elasticsearch migration, the stringId property was removed from ElasticCase and ElasticTask, with the id property now containing the MongoDB case/task object ID directly. This consolidation eliminates redundancy and simplifies the data model.
📚 Learning: 2025-10-20T11:44:44.907Z
Learnt from: machacjozef
Repo: netgrif/application-engine PR: 367
File: application-engine/src/main/resources/application.yaml:24-24
Timestamp: 2025-10-20T11:44:44.907Z
Learning: In the netgrif/application-engine project, the correction of the Elasticsearch task index name from "_taks" to "_task" in application.yaml was approved by maintainer machacjozef, indicating that any data migration concerns for this typo fix are handled separately or not applicable to their deployment scenario.
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticQueueManager.java
📚 Learning: 2025-11-04T11:47:03.883Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 378
File: nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/ElasticCase.java:16-16
Timestamp: 2025-11-04T11:47:03.883Z
Learning: In the netgrif/application-engine project, ElasticsearchRunner is responsible for creating Elasticsearch indexes automatically based on configuration. Spring Data's automatic index creation via the Document annotation's createIndex attribute should be disabled (set to false) to prevent conflicts with this centralized index management approach.
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticQueueManager.java
🧬 Code graph analysis (1)
application-engine/src/main/java/com/netgrif/application/engine/configuration/properties/DataConfigurationProperties.java (1)
application-engine/src/main/java/com/netgrif/application/engine/configuration/properties/SecurityConfigurationProperties.java (10)
Data(18-451)Data(117-129)Data(136-180)Data(186-195)Data(197-280)Data(249-279)Data(286-325)Data(327-346)Data(352-450)Data(418-449)
⏰ 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: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: Build
🔇 Additional comments (3)
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticQueueManager.java (3)
50-59: LGTM!The constructor properly initializes all components with thread-safe data structures (ConcurrentLinkedDeque, AtomicReference) and configurable properties. The scheduled thread pool size is configurable via properties, allowing deployment-specific tuning.
89-92: LGTM!The push method correctly adds elements to the thread-safe queue and resets the flush timer. Simple and effective.
99-122: LGTM!The flush method correctly handles batching, error recovery, and synchronization. The try-catch block ensures failed batches are re-queued, preventing data loss. Using
RefreshPolicy.WAIT_UNTILensures indexed documents are immediately searchable, which is appropriate for this use case.
...ava/com/netgrif/application/engine/configuration/properties/DataConfigurationProperties.java
Show resolved
Hide resolved
...ava/com/netgrif/application/engine/configuration/properties/DataConfigurationProperties.java
Show resolved
Hide resolved
Introduce a new `queue.delay` parameter in the Elasticsearch configuration within `application-test.yaml`. This ensures better control over queue processing behavior during test execution.
Introduced validation annotations (@Valid and @min) for queue-related properties to enforce valid configurations. Added a RefreshPolicy property with a default value of NONE to enhance control over Elasticsearch refresh behavior. These changes improve reliability and configurability of the queue settings.
Replaced ElasticsearchTemplate with ElasticsearchClient for bulk operations, improving efficiency and consistency. Updated ElasticQueueManager and related services to handle BulkOperation objects, eliminating deprecated methods and simplifying queue handling logic. Adjusted indexing and deletion logic to align with the new approach.
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
application-engine/src/main/java/com/netgrif/application/engine/configuration/ElasticServiceConfiguration.java(3 hunks)application-engine/src/main/java/com/netgrif/application/engine/configuration/properties/DataConfigurationProperties.java(3 hunks)application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticCaseService.java(3 hunks)application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticQueueManager.java(1 hunks)nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.java(1 hunks)nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/ElasticCase.java(2 hunks)
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: machacjozef
Repo: netgrif/application-engine PR: 367
File: application-engine/src/main/resources/application.yaml:24-24
Timestamp: 2025-10-20T11:44:44.907Z
Learning: In the netgrif/application-engine project, the correction of the Elasticsearch task index name from "_taks" to "_task" in application.yaml was approved by maintainer machacjozef, indicating that any data migration concerns for this typo fix are handled separately or not applicable to their deployment scenario.
📚 Learning: 2025-11-04T11:47:03.897Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 378
File: nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/ElasticCase.java:16-16
Timestamp: 2025-11-04T11:47:03.897Z
Learning: In the netgrif/application-engine project, ElasticsearchRunner is responsible for creating Elasticsearch indexes automatically based on configuration. Spring Data's automatic index creation via the Document annotation's createIndex attribute should be disabled (set to false) to prevent conflicts with this centralized index management approach.
Applied to files:
nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/ElasticCase.javaapplication-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticCaseService.javaapplication-engine/src/main/java/com/netgrif/application/engine/configuration/ElasticServiceConfiguration.javaapplication-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticQueueManager.java
📚 Learning: 2025-08-19T20:13:40.087Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 339
File: nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/CaseField.java:16-16
Timestamp: 2025-08-19T20:13:40.087Z
Learning: In CaseField.java, fulltextValue is mapped as a keyword field type in Elasticsearch (for exact matches, filtering, aggregations), while the separate caseValue field serves different Elasticsearch query requirements, allowing the system to support multiple query patterns on the same data through different field mappings.
Applied to files:
nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/ElasticCase.javaapplication-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticCaseService.javanae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.java
📚 Learning: 2025-08-19T20:07:15.621Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 339
File: application-engine/src/test/groovy/com/netgrif/application/engine/filters/FilterImportExportTest.groovy:341-341
Timestamp: 2025-08-19T20:07:15.621Z
Learning: In the Elasticsearch migration, the stringId property was removed from ElasticCase and ElasticTask, with the id property now containing the MongoDB case/task object ID directly. This consolidation eliminates redundancy and simplifies the data model.
Applied to files:
nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/ElasticCase.javaapplication-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticCaseService.javanae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.java
📚 Learning: 2025-08-19T20:07:43.748Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 339
File: nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/CaseField.java:16-16
Timestamp: 2025-08-19T20:07:43.748Z
Learning: In CaseField.java, the separate caseValue field (List<String>) is intentionally maintained alongside fulltextValue for specific Elasticsearch query requirements, rather than being derived on-the-fly from fulltextValue.
Applied to files:
nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/ElasticCase.javanae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.java
📚 Learning: 2025-10-20T11:44:44.907Z
Learnt from: machacjozef
Repo: netgrif/application-engine PR: 367
File: application-engine/src/main/resources/application.yaml:24-24
Timestamp: 2025-10-20T11:44:44.907Z
Learning: In the netgrif/application-engine project, the correction of the Elasticsearch task index name from "_taks" to "_task" in application.yaml was approved by maintainer machacjozef, indicating that any data migration concerns for this typo fix are handled separately or not applicable to their deployment scenario.
Applied to files:
nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/ElasticCase.javaapplication-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticCaseService.javaapplication-engine/src/main/java/com/netgrif/application/engine/configuration/ElasticServiceConfiguration.javaapplication-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticQueueManager.java
📚 Learning: 2025-07-29T17:19:18.300Z
Learnt from: tuplle
Repo: netgrif/application-engine PR: 331
File: application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticPetriNetService.java:45-46
Timestamp: 2025-07-29T17:19:18.300Z
Learning: In ElasticPetriNetService class, petriNetService is properly initialized using Lazy setter injection rather than constructor injection. This pattern with Lazy Autowired setter methods is commonly used in Spring to resolve circular dependencies and is a valid alternative to constructor injection.
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticCaseService.javaapplication-engine/src/main/java/com/netgrif/application/engine/configuration/ElasticServiceConfiguration.java
📚 Learning: 2025-08-20T07:27:02.660Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 339
File: application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticTaskQueueManager.java:38-45
Timestamp: 2025-08-20T07:27:02.660Z
Learning: When reviewing ElasticTaskQueueManager changes, task.getTask().getId() returns the document identifier while task.getTaskId() returns the business task identifier. The queue operations should use consistent identifiers throughout the lifecycle (scheduling, processing, cleanup).
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticCaseService.java
🧬 Code graph analysis (2)
nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/ElasticCase.java (1)
nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/workflow/domain/Task.java (1)
Document(23-59)
application-engine/src/main/java/com/netgrif/application/engine/configuration/properties/DataConfigurationProperties.java (1)
application-engine/src/main/java/com/netgrif/application/engine/configuration/properties/SecurityConfigurationProperties.java (10)
Data(18-451)Data(117-129)Data(136-180)Data(186-195)Data(197-280)Data(249-279)Data(286-325)Data(327-346)Data(352-450)Data(418-449)
⏰ 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). (4)
- GitHub Check: Test
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
🔇 Additional comments (10)
nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/ElasticCase.java (1)
26-26: LGTM: Class metadata field follows separation of concerns.The
_classfield provides a placeholder for runtime class information at the persistence layer. The correspondingget_class()accessor in the Spring adapter (nae-spring-core-adapter) returns the runtime class name, maintaining proper separation between domain objects and persistence concerns.nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/ElasticCase.java (2)
16-16: LGTM: Disabled automatic index creation per project convention.As confirmed in learnings,
createIndex = falseis intentional—ElasticsearchRunnercentrally manages index creation to avoid conflicts with Spring Data's annotation-driven creation.Based on learnings.
146-149: LGTM: Runtime class accessor provides type metadata.The
get_class()method exposes runtime class information as a Keyword field, suitable for exact matching and filtering in Elasticsearch. The implementation correctly usesthis.getClass().getName()to capture the fully qualified class name.application-engine/src/main/java/com/netgrif/application/engine/configuration/properties/DataConfigurationProperties.java (2)
525-534: LGTM: Queue properties properly configured with validation.The
queuefield is correctly annotated with@Validfor nested validation, follows the same pattern as thebatchfield, and includes comprehensive documentation explaining its purpose.
652-694: LGTM: Well-designed queue properties with appropriate validation.The
QueuePropertiesclass is well-structured with:
- Appropriate
@Min(1)constraints on numeric fields to prevent invalid configurations- Sensible defaults (pool size: 10, delay: 150ms, max queue size: 400)
- Clear documentation for each property
RefreshPolicy.NONEdefault optimizes bulk operation performanceapplication-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticQueueManager.java (1)
128-138: LGTM: Timer reset correctly prevents stalling under steady traffic.The
resetTimer()implementation properly addresses the concern raised in previous reviews. By usingupdateAndGet()and only scheduling a new flush when no active task exists, it ensures:
- Periodic flushing occurs even under continuous load
- Existing scheduled tasks are not cancelled prematurely
- Shutdown state is respected
application-engine/src/main/java/com/netgrif/application/engine/configuration/ElasticServiceConfiguration.java (1)
62-72: LGTM: Bean configurations properly wired with new dependencies.Both
elasticCaseService()andreindexingTaskElasticCaseService()correctly pass all required dependencies to theElasticCaseServiceconstructor, maintaining proper separation between primary and reindexing workflows through different executors.Also applies to: 83-93
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticCaseService.java (3)
65-84: LGTM: Constructor properly initializes queue-based indexing.The explicit constructor correctly:
- Uses
@Lazyinjection forpetriNetServiceandworkflowServiceto handle circular dependencies (per project patterns)- Initializes separate queue managers for index and delete operations
- Shares configuration and index name between both queue managers
Note: The
elasticsearchTemplatepassed to queue managers is unused (see separate comment onElasticQueueManager).
87-90: LGTM: Deletion properly queued with accurate logging.The
remove()method correctly:
- Enqueues deletion via
BulkOperation.delete- Specifies the correct index and document ID
- Logs "queued for deletion" to accurately reflect the asynchronous nature
101-120: No race condition evidence found in listener implementations.Event publishing timing is asynchronous as described, but extensive search revealed no listener implementations actually query Elasticsearch immediately upon receiving IndexCaseEvent. The dispatch mechanism forwards events to registered listeners via
onEvent()oronAsyncEvent()methods—it doesn't perform queries itself. Without identifying listeners that query ES, the stated race condition cannot be verified.The 150ms queue delay is appropriately configured as default, and event forwarding is decoupled from queue processing, which is by design.
.../src/main/java/com/netgrif/application/engine/configuration/ElasticServiceConfiguration.java
Show resolved
Hide resolved
...-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticCaseService.java
Outdated
Show resolved
Hide resolved
...engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticQueueManager.java
Show resolved
Hide resolved
...engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticQueueManager.java
Show resolved
Hide resolved
Removed unnecessary parameters from ElasticQueueManager constructor and updated related instantiations. Simplified timer reset logic, added scheduler shutdown, and cleaned up unused imports and repositories in ElasticServiceConfiguration.
Added a condition to flush the queue when its size exceeds the maximum threshold, ensuring more efficient management. The timer reset now occurs only if the flush is not triggered, improving resource utilization and robustness.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticCaseService.java (1)
118-120: Don’t fireIndexCaseEventbefore the document exists in Elasticsearch.We now enqueue the write, so publishing
IndexCaseEventimmediately advertises a case that may not be searchable for seconds (or indefinitely on failure). Downstream listeners will see a false positive. Defer the event until the queue confirms a successful bulk write (e.g., via a callback fromElasticQueueManager) or rename the event to make the eventual consistency explicit.- log.debug("[{}]: Case \"{}\" queued for indexing", useCase.getId(), useCase.getTitle()); - publisher.publishEvent(new IndexCaseEvent(useCase)); + log.debug("[{}]: Case \"{}\" queued for indexing", useCase.getId(), useCase.getTitle()); + // Publish the IndexCaseEvent from the queue manager once the bulk write succeeds.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
application-engine/src/main/java/com/netgrif/application/engine/configuration/ElasticServiceConfiguration.java(3 hunks)application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticCaseService.java(3 hunks)application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticQueueManager.java(1 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: machacjozef
Repo: netgrif/application-engine PR: 367
File: application-engine/src/main/resources/application.yaml:24-24
Timestamp: 2025-10-20T11:44:44.907Z
Learning: In the netgrif/application-engine project, the correction of the Elasticsearch task index name from "_taks" to "_task" in application.yaml was approved by maintainer machacjozef, indicating that any data migration concerns for this typo fix are handled separately or not applicable to their deployment scenario.
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 339
File: application-engine/src/test/groovy/com/netgrif/application/engine/filters/FilterImportExportTest.groovy:341-341
Timestamp: 2025-08-19T20:07:15.621Z
Learning: In the Elasticsearch migration, the stringId property was removed from ElasticCase and ElasticTask, with the id property now containing the MongoDB case/task object ID directly. This consolidation eliminates redundancy and simplifies the data model.
📚 Learning: 2025-07-29T17:19:18.300Z
Learnt from: tuplle
Repo: netgrif/application-engine PR: 331
File: application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticPetriNetService.java:45-46
Timestamp: 2025-07-29T17:19:18.300Z
Learning: In ElasticPetriNetService class, petriNetService is properly initialized using Lazy setter injection rather than constructor injection. This pattern with Lazy Autowired setter methods is commonly used in Spring to resolve circular dependencies and is a valid alternative to constructor injection.
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/configuration/ElasticServiceConfiguration.javaapplication-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticCaseService.java
📚 Learning: 2025-11-04T11:47:03.897Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 378
File: nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/ElasticCase.java:16-16
Timestamp: 2025-11-04T11:47:03.897Z
Learning: In the netgrif/application-engine project, ElasticsearchRunner is responsible for creating Elasticsearch indexes automatically based on configuration. Spring Data's automatic index creation via the Document annotation's createIndex attribute should be disabled (set to false) to prevent conflicts with this centralized index management approach.
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/configuration/ElasticServiceConfiguration.javaapplication-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticQueueManager.javaapplication-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticCaseService.java
📚 Learning: 2025-08-19T20:07:15.621Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 339
File: application-engine/src/test/groovy/com/netgrif/application/engine/filters/FilterImportExportTest.groovy:341-341
Timestamp: 2025-08-19T20:07:15.621Z
Learning: In the Elasticsearch migration, the stringId property was removed from ElasticCase and ElasticTask, with the id property now containing the MongoDB case/task object ID directly. This consolidation eliminates redundancy and simplifies the data model.
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/configuration/ElasticServiceConfiguration.javaapplication-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticCaseService.java
📚 Learning: 2025-10-20T11:44:44.907Z
Learnt from: machacjozef
Repo: netgrif/application-engine PR: 367
File: application-engine/src/main/resources/application.yaml:24-24
Timestamp: 2025-10-20T11:44:44.907Z
Learning: In the netgrif/application-engine project, the correction of the Elasticsearch task index name from "_taks" to "_task" in application.yaml was approved by maintainer machacjozef, indicating that any data migration concerns for this typo fix are handled separately or not applicable to their deployment scenario.
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/configuration/ElasticServiceConfiguration.javaapplication-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticQueueManager.javaapplication-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticCaseService.java
📚 Learning: 2025-08-20T07:27:02.660Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 339
File: application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticTaskQueueManager.java:38-45
Timestamp: 2025-08-20T07:27:02.660Z
Learning: When reviewing ElasticTaskQueueManager changes, task.getTask().getId() returns the document identifier while task.getTaskId() returns the business task identifier. The queue operations should use consistent identifiers throughout the lifecycle (scheduling, processing, cleanup).
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticCaseService.java
📚 Learning: 2025-08-19T20:13:40.087Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 339
File: nae-object-library/src/main/java/com/netgrif/application/engine/objects/elastic/domain/CaseField.java:16-16
Timestamp: 2025-08-19T20:13:40.087Z
Learning: In CaseField.java, fulltextValue is mapped as a keyword field type in Elasticsearch (for exact matches, filtering, aggregations), while the separate caseValue field serves different Elasticsearch query requirements, allowing the system to support multiple query patterns on the same data through different field mappings.
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticCaseService.java
⏰ 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). (4)
- GitHub Check: Test
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
...-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticCaseService.java
Outdated
Show resolved
Hide resolved
...-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticCaseService.java
Outdated
Show resolved
Hide resolved
...engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticQueueManager.java
Show resolved
Hide resolved
...engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticQueueManager.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticQueueManager.java (2)
142-142: Use configured refresh policy instead of hardcoded value.Line 142 hardcodes
refresh(Refresh.False), ignoring thequeueProperties.getRefreshPolicy()configuration. This prevents operators from tuning refresh behavior based on their use case (e.g., usingRefresh.WaitForfor test environments orRefresh.Truewhen immediate visibility is required).The configuration uses Spring Data Elasticsearch's
RefreshPolicyenum, but the Elasticsearch client requiresco.elastic.clients.elasticsearch._types.Refresh. Add a converter method and apply this diff:+ private Refresh convertRefreshPolicy(org.springframework.data.elasticsearch.core.RefreshPolicy policy) { + return switch (policy) { + case IMMEDIATE -> Refresh.True; + case WAIT_UNTIL -> Refresh.WaitFor; + case NONE -> Refresh.False; + }; + } + private synchronized void flush() { if (queue.isEmpty()) { return; } List<BulkOperationWrapper> batch = new ArrayList<>(); while (!queue.isEmpty() && batch.size() < queueProperties.getMaxBatchSize()) { batch.add(queue.poll()); } String uuid = UUID.randomUUID().toString(); try { log.debug("Index started with batch size: {} and id: {}", batch.size(), uuid); - elasticsearchClient.bulk(new BulkRequest.Builder().operations(batch.stream().map(BulkOperationWrapper::getOperation).toList()).refresh(Refresh.False).build()); + elasticsearchClient.bulk(new BulkRequest.Builder() + .operations(batch.stream().map(BulkOperationWrapper::getOperation).toList()) + .refresh(convertRefreshPolicy(queueProperties.getRefreshPolicy())) + .build()); log.debug("Index finished with batch size: {} and id: {}", batch.size(), uuid);
145-151: Move event publishing out of finally block to prevent premature event publication.The
finallyblock at line 150 executespublishEventsOfBatch(batch)regardless of whether the bulk operation succeeded or failed. This causes events to be published for operations that failed and were re-queued in the catch block (line 146), leading to:
- Events published before operations actually succeed
- Potential duplicate or inconsistent event notifications when the batch is retried
- Loss of event/operation ordering guarantees
Apply this diff to only publish events after successful bulk operations:
String uuid = UUID.randomUUID().toString(); try { log.debug("Index started with batch size: {} and id: {}", batch.size(), uuid); elasticsearchClient.bulk(new BulkRequest.Builder().operations(batch.stream().map(BulkOperationWrapper::getOperation).toList()).refresh(Refresh.False).build()); log.debug("Index finished with batch size: {} and id: {}", batch.size(), uuid); - checkQueue(); } catch (Exception e) { queue.addAll(batch); resetTimer(); log.error("Index failed with batch size: {} and id: {}", batch.size(), uuid, e); - } finally { + return; + } + + try { publishEventsOfBatch(batch); + } catch (Exception e) { + log.error("Event publishing failed for batch id: {}", uuid, e); } + + checkQueue(); }This ensures events are only published after the bulk operation succeeds, and event publishing failures don't affect the bulk operation's success status.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticQueueManager.java(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: machacjozef
Repo: netgrif/application-engine PR: 367
File: application-engine/src/main/resources/application.yaml:24-24
Timestamp: 2025-10-20T11:44:44.907Z
Learning: In the netgrif/application-engine project, the correction of the Elasticsearch task index name from "_taks" to "_task" in application.yaml was approved by maintainer machacjozef, indicating that any data migration concerns for this typo fix are handled separately or not applicable to their deployment scenario.
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 339
File: application-engine/src/test/groovy/com/netgrif/application/engine/filters/FilterImportExportTest.groovy:341-341
Timestamp: 2025-08-19T20:07:15.621Z
Learning: In the Elasticsearch migration, the stringId property was removed from ElasticCase and ElasticTask, with the id property now containing the MongoDB case/task object ID directly. This consolidation eliminates redundancy and simplifies the data model.
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 378
File: nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/ElasticCase.java:16-16
Timestamp: 2025-11-04T11:47:03.897Z
Learning: In the netgrif/application-engine project, ElasticsearchRunner is responsible for creating Elasticsearch indexes automatically based on configuration. Spring Data's automatic index creation via the Document annotation's createIndex attribute should be disabled (set to false) to prevent conflicts with this centralized index management approach.
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 339
File: application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticTaskQueueManager.java:38-45
Timestamp: 2025-08-20T07:27:02.660Z
Learning: When reviewing ElasticTaskQueueManager changes, task.getTask().getId() returns the document identifier while task.getTaskId() returns the business task identifier. The queue operations should use consistent identifiers throughout the lifecycle (scheduling, processing, cleanup).
📚 Learning: 2025-10-20T11:44:44.907Z
Learnt from: machacjozef
Repo: netgrif/application-engine PR: 367
File: application-engine/src/main/resources/application.yaml:24-24
Timestamp: 2025-10-20T11:44:44.907Z
Learning: In the netgrif/application-engine project, the correction of the Elasticsearch task index name from "_taks" to "_task" in application.yaml was approved by maintainer machacjozef, indicating that any data migration concerns for this typo fix are handled separately or not applicable to their deployment scenario.
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticQueueManager.java
📚 Learning: 2025-11-04T11:47:03.897Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 378
File: nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/ElasticCase.java:16-16
Timestamp: 2025-11-04T11:47:03.897Z
Learning: In the netgrif/application-engine project, ElasticsearchRunner is responsible for creating Elasticsearch indexes automatically based on configuration. Spring Data's automatic index creation via the Document annotation's createIndex attribute should be disabled (set to false) to prevent conflicts with this centralized index management approach.
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticQueueManager.java
⏰ 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). (12)
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: Test
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
...engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticQueueManager.java
Show resolved
Hide resolved
Moved `publishEventsOfBatch()` outside the `finally` block to ensure it is not called when an exception occurs. This improves handling of failed indexing batches and prevents unintended event publishing.
Replaced usage of `RefreshPolicy` with `Refresh` in `ElasticQueueManager` and aligned the default refresh policy to `Refresh.False`. This ensures consistency and improves clarity in refresh policy configuration for bulk operations.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
application-engine/src/main/java/com/netgrif/application/engine/configuration/properties/DataConfigurationProperties.java(4 hunks)application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticQueueManager.java(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: machacjozef
Repo: netgrif/application-engine PR: 367
File: application-engine/src/main/resources/application.yaml:24-24
Timestamp: 2025-10-20T11:44:44.907Z
Learning: In the netgrif/application-engine project, the correction of the Elasticsearch task index name from "_taks" to "_task" in application.yaml was approved by maintainer machacjozef, indicating that any data migration concerns for this typo fix are handled separately or not applicable to their deployment scenario.
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 339
File: application-engine/src/test/groovy/com/netgrif/application/engine/filters/FilterImportExportTest.groovy:341-341
Timestamp: 2025-08-19T20:07:15.621Z
Learning: In the Elasticsearch migration, the stringId property was removed from ElasticCase and ElasticTask, with the id property now containing the MongoDB case/task object ID directly. This consolidation eliminates redundancy and simplifies the data model.
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 378
File: nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/ElasticCase.java:16-16
Timestamp: 2025-11-04T11:47:03.897Z
Learning: In the netgrif/application-engine project, ElasticsearchRunner is responsible for creating Elasticsearch indexes automatically based on configuration. Spring Data's automatic index creation via the Document annotation's createIndex attribute should be disabled (set to false) to prevent conflicts with this centralized index management approach.
📚 Learning: 2025-10-20T11:44:44.907Z
Learnt from: machacjozef
Repo: netgrif/application-engine PR: 367
File: application-engine/src/main/resources/application.yaml:24-24
Timestamp: 2025-10-20T11:44:44.907Z
Learning: In the netgrif/application-engine project, the correction of the Elasticsearch task index name from "_taks" to "_task" in application.yaml was approved by maintainer machacjozef, indicating that any data migration concerns for this typo fix are handled separately or not applicable to their deployment scenario.
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticQueueManager.javaapplication-engine/src/main/java/com/netgrif/application/engine/configuration/properties/DataConfigurationProperties.java
📚 Learning: 2025-11-04T11:47:03.897Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 378
File: nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/ElasticCase.java:16-16
Timestamp: 2025-11-04T11:47:03.897Z
Learning: In the netgrif/application-engine project, ElasticsearchRunner is responsible for creating Elasticsearch indexes automatically based on configuration. Spring Data's automatic index creation via the Document annotation's createIndex attribute should be disabled (set to false) to prevent conflicts with this centralized index management approach.
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticQueueManager.java
⏰ 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). (8)
- GitHub Check: Test
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
...engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticQueueManager.java
Show resolved
Hide resolved
Updated the condition to use `getMaxBatchSize` instead of `getMaxQueueSize` to align with batch configuration logic. Enhanced error logging for failed bulk operations to provide clearer context and describe retry mechanisms.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticQueueManager.java(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: machacjozef
Repo: netgrif/application-engine PR: 367
File: application-engine/src/main/resources/application.yaml:24-24
Timestamp: 2025-10-20T11:44:44.907Z
Learning: In the netgrif/application-engine project, the correction of the Elasticsearch task index name from "_taks" to "_task" in application.yaml was approved by maintainer machacjozef, indicating that any data migration concerns for this typo fix are handled separately or not applicable to their deployment scenario.
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 339
File: application-engine/src/test/groovy/com/netgrif/application/engine/filters/FilterImportExportTest.groovy:341-341
Timestamp: 2025-08-19T20:07:15.621Z
Learning: In the Elasticsearch migration, the stringId property was removed from ElasticCase and ElasticTask, with the id property now containing the MongoDB case/task object ID directly. This consolidation eliminates redundancy and simplifies the data model.
📚 Learning: 2025-10-20T11:44:44.907Z
Learnt from: machacjozef
Repo: netgrif/application-engine PR: 367
File: application-engine/src/main/resources/application.yaml:24-24
Timestamp: 2025-10-20T11:44:44.907Z
Learning: In the netgrif/application-engine project, the correction of the Elasticsearch task index name from "_taks" to "_task" in application.yaml was approved by maintainer machacjozef, indicating that any data migration concerns for this typo fix are handled separately or not applicable to their deployment scenario.
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticQueueManager.java
📚 Learning: 2025-11-04T11:47:03.897Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 378
File: nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/ElasticCase.java:16-16
Timestamp: 2025-11-04T11:47:03.897Z
Learning: In the netgrif/application-engine project, ElasticsearchRunner is responsible for creating Elasticsearch indexes automatically based on configuration. Spring Data's automatic index creation via the Document annotation's createIndex attribute should be disabled (set to false) to prevent conflicts with this centralized index management approach.
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticQueueManager.java
📚 Learning: 2025-08-20T07:24:22.566Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 339
File: application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticTaskQueueManager.java:38-45
Timestamp: 2025-08-20T07:24:22.566Z
Learning: When renczesstefan indicates a conversation is resolved after conflicting guidance, respect their decision and don't push for further changes.
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticQueueManager.java
⏰ 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: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: Build
🔇 Additional comments (7)
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticQueueManager.java (7)
1-49: LGTM! Clean class structure and comprehensive documentation.The imports, field declarations, and class-level Javadoc are well-organized. The use of
BlockingQueue,AtomicReference, andScheduledExecutorServicedemonstrates proper thread-safety considerations.
59-68: LGTM! Constructor properly initializes bounded queue and dependencies.The use of
LinkedBlockingDequewithmaxQueueSizeensures bounded memory usage, and all required dependencies are correctly wired.
80-96: LGTM! Shutdown sequence is correct.The method properly cancels pending tasks, shuts down the scheduler before flushing, and handles the remaining queue gracefully with appropriate timeout handling.
129-152: LGTM! Flush implementation is correct.The method properly handles batch processing with appropriate error handling. The decision not to requeue failed batches aligns with your stated fallback mechanisms (scheduled indexing and manual reindex endpoints).
162-172: LGTM! Timer reset is properly synchronized.The
synchronizedmodifier correctly prevents the race condition where multiple threads could orphan scheduled tasks. The shutdown check ensures no tasks are scheduled on a terminated executor.
181-187: LGTM! Queue check logic is correct.The method appropriately triggers immediate flush when a full batch is available, or resets the timer otherwise. This provides efficient batch processing under varying load conditions.
198-201: LGTM! Event publishing is clean and correct.The method properly filters for operations with publishable events and publishes them using the injected
ApplicationEventPublisher.
...engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticQueueManager.java
Show resolved
Hide resolved
...engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticQueueManager.java
Show resolved
Hide resolved
The javadoc for the push method incorrectly mentioned an exception that is no longer thrown. This cleanup improves code readability and consistency by removing outdated or misleading information.
The javadoc for the push method incorrectly mentioned an exception that is no longer thrown. This cleanup improves code readability and consistency by removing outdated or misleading information.
The JavaDoc for the `flush` method had an unnecessary annotation about an exception that is not thrown. This update corrects the formatting and removes misleading information about a nonexistent exception.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticQueueManager.java(1 hunks)
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: machacjozef
Repo: netgrif/application-engine PR: 367
File: application-engine/src/main/resources/application.yaml:24-24
Timestamp: 2025-10-20T11:44:44.907Z
Learning: In the netgrif/application-engine project, the correction of the Elasticsearch task index name from "_taks" to "_task" in application.yaml was approved by maintainer machacjozef, indicating that any data migration concerns for this typo fix are handled separately or not applicable to their deployment scenario.
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 339
File: application-engine/src/test/groovy/com/netgrif/application/engine/filters/FilterImportExportTest.groovy:341-341
Timestamp: 2025-08-19T20:07:15.621Z
Learning: In the Elasticsearch migration, the stringId property was removed from ElasticCase and ElasticTask, with the id property now containing the MongoDB case/task object ID directly. This consolidation eliminates redundancy and simplifies the data model.
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 378
File: application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticQueueManager.java:145-149
Timestamp: 2025-11-07T13:11:20.604Z
Learning: In the netgrif/application-engine project, failed Elasticsearch bulk operations in ElasticQueueManager should not be retried or re-queued within the flush() method. The system relies on scheduled reindexing and manual reindex endpoints as fallback mechanisms for failed operations, making in-method retries unnecessary and potentially problematic (could cause deadlocks, capacity issues, or infinite loops with bad data).
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 339
File: application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticTaskQueueManager.java:38-45
Timestamp: 2025-08-20T07:27:02.660Z
Learning: When reviewing ElasticTaskQueueManager changes, task.getTask().getId() returns the document identifier while task.getTaskId() returns the business task identifier. The queue operations should use consistent identifiers throughout the lifecycle (scheduling, processing, cleanup).
📚 Learning: 2025-11-07T13:11:20.604Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 378
File: application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticQueueManager.java:145-149
Timestamp: 2025-11-07T13:11:20.604Z
Learning: In the netgrif/application-engine project, failed Elasticsearch bulk operations in ElasticQueueManager should not be retried or re-queued within the flush() method. The system relies on scheduled reindexing and manual reindex endpoints as fallback mechanisms for failed operations, making in-method retries unnecessary and potentially problematic (could cause deadlocks, capacity issues, or infinite loops with bad data).
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticQueueManager.java
📚 Learning: 2025-10-20T11:44:44.907Z
Learnt from: machacjozef
Repo: netgrif/application-engine PR: 367
File: application-engine/src/main/resources/application.yaml:24-24
Timestamp: 2025-10-20T11:44:44.907Z
Learning: In the netgrif/application-engine project, the correction of the Elasticsearch task index name from "_taks" to "_task" in application.yaml was approved by maintainer machacjozef, indicating that any data migration concerns for this typo fix are handled separately or not applicable to their deployment scenario.
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticQueueManager.java
📚 Learning: 2025-11-04T11:47:03.897Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 378
File: nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/ElasticCase.java:16-16
Timestamp: 2025-11-04T11:47:03.897Z
Learning: In the netgrif/application-engine project, ElasticsearchRunner is responsible for creating Elasticsearch indexes automatically based on configuration. Spring Data's automatic index creation via the Document annotation's createIndex attribute should be disabled (set to false) to prevent conflicts with this centralized index management approach.
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticQueueManager.java
📚 Learning: 2025-08-20T07:24:22.566Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 339
File: application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticTaskQueueManager.java:38-45
Timestamp: 2025-08-20T07:24:22.566Z
Learning: When renczesstefan indicates a conversation is resolved after conflicting guidance, respect their decision and don't push for further changes.
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticQueueManager.java
📚 Learning: 2025-08-20T07:27:02.660Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 339
File: application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticTaskQueueManager.java:38-45
Timestamp: 2025-08-20T07:27:02.660Z
Learning: When reviewing ElasticTaskQueueManager changes, task.getTask().getId() returns the document identifier while task.getTaskId() returns the business task identifier. The queue operations should use consistent identifiers throughout the lifecycle (scheduling, processing, cleanup).
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticQueueManager.java
⏰ 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). (12)
- GitHub Check: Test
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
...engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticQueueManager.java
Outdated
Show resolved
Hide resolved
The JavaDoc for the `flush` method had an unnecessary annotation about an exception that is not thrown. This update corrects the formatting and removes misleading information about a nonexistent exception.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticQueueManager.java(1 hunks)
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: machacjozef
Repo: netgrif/application-engine PR: 367
File: application-engine/src/main/resources/application.yaml:24-24
Timestamp: 2025-10-20T11:44:44.907Z
Learning: In the netgrif/application-engine project, the correction of the Elasticsearch task index name from "_taks" to "_task" in application.yaml was approved by maintainer machacjozef, indicating that any data migration concerns for this typo fix are handled separately or not applicable to their deployment scenario.
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 378
File: application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticQueueManager.java:145-149
Timestamp: 2025-11-07T13:11:20.604Z
Learning: In the netgrif/application-engine project, failed Elasticsearch bulk operations in ElasticQueueManager should not be retried or re-queued within the flush() method. The system relies on scheduled reindexing and manual reindex endpoints as fallback mechanisms for failed operations, making in-method retries unnecessary and potentially problematic (could cause deadlocks, capacity issues, or infinite loops with bad data).
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 339
File: application-engine/src/test/groovy/com/netgrif/application/engine/filters/FilterImportExportTest.groovy:341-341
Timestamp: 2025-08-19T20:07:15.621Z
Learning: In the Elasticsearch migration, the stringId property was removed from ElasticCase and ElasticTask, with the id property now containing the MongoDB case/task object ID directly. This consolidation eliminates redundancy and simplifies the data model.
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 378
File: nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/ElasticCase.java:16-16
Timestamp: 2025-11-04T11:47:03.897Z
Learning: In the netgrif/application-engine project, ElasticsearchRunner is responsible for creating Elasticsearch indexes automatically based on configuration. Spring Data's automatic index creation via the Document annotation's createIndex attribute should be disabled (set to false) to prevent conflicts with this centralized index management approach.
📚 Learning: 2025-11-07T13:11:20.604Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 378
File: application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticQueueManager.java:145-149
Timestamp: 2025-11-07T13:11:20.604Z
Learning: In the netgrif/application-engine project, failed Elasticsearch bulk operations in ElasticQueueManager should not be retried or re-queued within the flush() method. The system relies on scheduled reindexing and manual reindex endpoints as fallback mechanisms for failed operations, making in-method retries unnecessary and potentially problematic (could cause deadlocks, capacity issues, or infinite loops with bad data).
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticQueueManager.java
📚 Learning: 2025-10-20T11:44:44.907Z
Learnt from: machacjozef
Repo: netgrif/application-engine PR: 367
File: application-engine/src/main/resources/application.yaml:24-24
Timestamp: 2025-10-20T11:44:44.907Z
Learning: In the netgrif/application-engine project, the correction of the Elasticsearch task index name from "_taks" to "_task" in application.yaml was approved by maintainer machacjozef, indicating that any data migration concerns for this typo fix are handled separately or not applicable to their deployment scenario.
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticQueueManager.java
📚 Learning: 2025-11-04T11:47:03.897Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 378
File: nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/ElasticCase.java:16-16
Timestamp: 2025-11-04T11:47:03.897Z
Learning: In the netgrif/application-engine project, ElasticsearchRunner is responsible for creating Elasticsearch indexes automatically based on configuration. Spring Data's automatic index creation via the Document annotation's createIndex attribute should be disabled (set to false) to prevent conflicts with this centralized index management approach.
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticQueueManager.java
📚 Learning: 2025-08-20T07:24:22.566Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 339
File: application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticTaskQueueManager.java:38-45
Timestamp: 2025-08-20T07:24:22.566Z
Learning: When renczesstefan indicates a conversation is resolved after conflicting guidance, respect their decision and don't push for further changes.
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticQueueManager.java
📚 Learning: 2025-08-20T07:27:02.660Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 339
File: application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticTaskQueueManager.java:38-45
Timestamp: 2025-08-20T07:27:02.660Z
Learning: When reviewing ElasticTaskQueueManager changes, task.getTask().getId() returns the document identifier while task.getTaskId() returns the business task identifier. The queue operations should use consistent identifiers throughout the lifecycle (scheduling, processing, cleanup).
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticQueueManager.java
⏰ 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). (4)
- GitHub Check: Test
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
🔇 Additional comments (6)
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticQueueManager.java (6)
17-48: LGTM - Well-documented and properly structured.The class-level documentation clearly explains the queue manager's responsibilities and key components. Field declarations follow best practices with final modifiers and appropriate thread-safe data structures (BlockingQueue, AtomicReference).
50-67: LGTM - Bounded queue properly configured.The constructor correctly initializes a bounded
LinkedBlockingDequewithmaxQueueSize, addressing memory safety concerns. All dependencies are properly injected and fields are initialized correctly.
98-115: LGTM - Queue management and backpressure correctly implemented.The blocking
put()provides natural backpressure when the queue reaches capacity. The timer reset logic (line 109) correctly usesmaxBatchSizeto stop debouncing when batches are ready to flush, as intended per your design discussions.
117-151: LGTM - Bulk operation handling correctly implemented.The synchronized method ensures thread safety. Error handling correctly avoids retrying failed operations, relying instead on scheduled reindexing as per the system's design. Event publishing occurs only after successful bulk operations (line 150), preventing duplicate indexing if event publishing fails.
Based on learnings.
153-171: LGTM - Timer reset properly synchronized.The
synchronizedmodifier prevents race conditions that could create orphaned scheduled tasks. The shutdown check (line 162) avoids scheduling on a terminated executor. This correctly implements the design discussed in previous reviews.
173-200: LGTM - Queue checking and event publishing implemented correctly.The
checkQueue()method provides aggressive draining when the queue remains large after a flush (lines 181-182), whilepublishEventsOfBatch()cleanly publishes events for successful operations using stream operations.
...engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticQueueManager.java
Show resolved
Hide resolved
Removed redundant `@throws` tag in the javadoc to improve clarity and adhere to current documentation standards. No functional changes were made in the code.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticQueueManager.java(1 hunks)
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: machacjozef
Repo: netgrif/application-engine PR: 367
File: application-engine/src/main/resources/application.yaml:24-24
Timestamp: 2025-10-20T11:44:44.907Z
Learning: In the netgrif/application-engine project, the correction of the Elasticsearch task index name from "_taks" to "_task" in application.yaml was approved by maintainer machacjozef, indicating that any data migration concerns for this typo fix are handled separately or not applicable to their deployment scenario.
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 378
File: application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticQueueManager.java:145-149
Timestamp: 2025-11-07T13:11:20.604Z
Learning: In the netgrif/application-engine project, failed Elasticsearch bulk operations in ElasticQueueManager should not be retried or re-queued within the flush() method. The system relies on scheduled reindexing and manual reindex endpoints as fallback mechanisms for failed operations, making in-method retries unnecessary and potentially problematic (could cause deadlocks, capacity issues, or infinite loops with bad data).
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 339
File: application-engine/src/test/groovy/com/netgrif/application/engine/filters/FilterImportExportTest.groovy:341-341
Timestamp: 2025-08-19T20:07:15.621Z
Learning: In the Elasticsearch migration, the stringId property was removed from ElasticCase and ElasticTask, with the id property now containing the MongoDB case/task object ID directly. This consolidation eliminates redundancy and simplifies the data model.
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 378
File: nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/ElasticCase.java:16-16
Timestamp: 2025-11-04T11:47:03.897Z
Learning: In the netgrif/application-engine project, ElasticsearchRunner is responsible for creating Elasticsearch indexes automatically based on configuration. Spring Data's automatic index creation via the Document annotation's createIndex attribute should be disabled (set to false) to prevent conflicts with this centralized index management approach.
📚 Learning: 2025-11-07T13:11:20.604Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 378
File: application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticQueueManager.java:145-149
Timestamp: 2025-11-07T13:11:20.604Z
Learning: In the netgrif/application-engine project, failed Elasticsearch bulk operations in ElasticQueueManager should not be retried or re-queued within the flush() method. The system relies on scheduled reindexing and manual reindex endpoints as fallback mechanisms for failed operations, making in-method retries unnecessary and potentially problematic (could cause deadlocks, capacity issues, or infinite loops with bad data).
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticQueueManager.java
📚 Learning: 2025-10-20T11:44:44.907Z
Learnt from: machacjozef
Repo: netgrif/application-engine PR: 367
File: application-engine/src/main/resources/application.yaml:24-24
Timestamp: 2025-10-20T11:44:44.907Z
Learning: In the netgrif/application-engine project, the correction of the Elasticsearch task index name from "_taks" to "_task" in application.yaml was approved by maintainer machacjozef, indicating that any data migration concerns for this typo fix are handled separately or not applicable to their deployment scenario.
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticQueueManager.java
📚 Learning: 2025-11-04T11:47:03.897Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 378
File: nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/ElasticCase.java:16-16
Timestamp: 2025-11-04T11:47:03.897Z
Learning: In the netgrif/application-engine project, ElasticsearchRunner is responsible for creating Elasticsearch indexes automatically based on configuration. Spring Data's automatic index creation via the Document annotation's createIndex attribute should be disabled (set to false) to prevent conflicts with this centralized index management approach.
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticQueueManager.java
📚 Learning: 2025-08-20T07:24:22.566Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 339
File: application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticTaskQueueManager.java:38-45
Timestamp: 2025-08-20T07:24:22.566Z
Learning: When renczesstefan indicates a conversation is resolved after conflicting guidance, respect their decision and don't push for further changes.
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticQueueManager.java
📚 Learning: 2025-08-20T07:27:02.660Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 339
File: application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticTaskQueueManager.java:38-45
Timestamp: 2025-08-20T07:27:02.660Z
Learning: When reviewing ElasticTaskQueueManager changes, task.getTask().getId() returns the document identifier while task.getTaskId() returns the business task identifier. The queue operations should use consistent identifiers throughout the lifecycle (scheduling, processing, cleanup).
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticQueueManager.java
⏰ 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). (8)
- GitHub Check: Test
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
...engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticQueueManager.java
Show resolved
Hide resolved
...engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticQueueManager.java
Outdated
Show resolved
Hide resolved
machacjozef
changed the base branch from
release/7.0.0-rev9
to
release/7.0.0-rev8
# Conflicts: # application-engine/pom.xml # nae-object-library/pom.xml # nae-spring-core-adapter/pom.xml # nae-user-ce/pom.xml # nae-user-common/pom.xml # pom.xml
Updated exception handling to log failures during batch event publishing. Clarified retry mechanisms in class documentation, specifying scheduled reindexing or manual intervention for failed bulk operations. This improves transparency and resilience in error scenarios.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticQueueManager.java(1 hunks)
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: machacjozef
Repo: netgrif/application-engine PR: 367
File: application-engine/src/main/resources/application.yaml:24-24
Timestamp: 2025-10-20T11:44:44.907Z
Learning: In the netgrif/application-engine project, the correction of the Elasticsearch task index name from "_taks" to "_task" in application.yaml was approved by maintainer machacjozef, indicating that any data migration concerns for this typo fix are handled separately or not applicable to their deployment scenario.
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 378
File: application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticQueueManager.java:145-149
Timestamp: 2025-11-07T13:11:20.622Z
Learning: In the netgrif/application-engine project, failed Elasticsearch bulk operations in ElasticQueueManager should not be retried or re-queued within the flush() method. The system relies on scheduled reindexing and manual reindex endpoints as fallback mechanisms for failed operations, making in-method retries unnecessary and potentially problematic (could cause deadlocks, capacity issues, or infinite loops with bad data).
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 339
File: application-engine/src/test/groovy/com/netgrif/application/engine/filters/FilterImportExportTest.groovy:341-341
Timestamp: 2025-08-19T20:07:15.621Z
Learning: In the Elasticsearch migration, the stringId property was removed from ElasticCase and ElasticTask, with the id property now containing the MongoDB case/task object ID directly. This consolidation eliminates redundancy and simplifies the data model.
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 378
File: nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/ElasticCase.java:16-16
Timestamp: 2025-11-04T11:47:03.897Z
Learning: In the netgrif/application-engine project, ElasticsearchRunner is responsible for creating Elasticsearch indexes automatically based on configuration. Spring Data's automatic index creation via the Document annotation's createIndex attribute should be disabled (set to false) to prevent conflicts with this centralized index management approach.
📚 Learning: 2025-11-07T13:11:20.622Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 378
File: application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticQueueManager.java:145-149
Timestamp: 2025-11-07T13:11:20.622Z
Learning: In the netgrif/application-engine project, failed Elasticsearch bulk operations in ElasticQueueManager should not be retried or re-queued within the flush() method. The system relies on scheduled reindexing and manual reindex endpoints as fallback mechanisms for failed operations, making in-method retries unnecessary and potentially problematic (could cause deadlocks, capacity issues, or infinite loops with bad data).
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticQueueManager.java
📚 Learning: 2025-10-20T11:44:44.907Z
Learnt from: machacjozef
Repo: netgrif/application-engine PR: 367
File: application-engine/src/main/resources/application.yaml:24-24
Timestamp: 2025-10-20T11:44:44.907Z
Learning: In the netgrif/application-engine project, the correction of the Elasticsearch task index name from "_taks" to "_task" in application.yaml was approved by maintainer machacjozef, indicating that any data migration concerns for this typo fix are handled separately or not applicable to their deployment scenario.
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticQueueManager.java
📚 Learning: 2025-11-04T11:47:03.897Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 378
File: nae-spring-core-adapter/src/main/java/com/netgrif/application/engine/adapter/spring/elastic/domain/ElasticCase.java:16-16
Timestamp: 2025-11-04T11:47:03.897Z
Learning: In the netgrif/application-engine project, ElasticsearchRunner is responsible for creating Elasticsearch indexes automatically based on configuration. Spring Data's automatic index creation via the Document annotation's createIndex attribute should be disabled (set to false) to prevent conflicts with this centralized index management approach.
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticQueueManager.java
📚 Learning: 2025-08-20T07:24:22.566Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 339
File: application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticTaskQueueManager.java:38-45
Timestamp: 2025-08-20T07:24:22.566Z
Learning: When renczesstefan indicates a conversation is resolved after conflicting guidance, respect their decision and don't push for further changes.
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticQueueManager.java
📚 Learning: 2025-08-20T07:27:02.660Z
Learnt from: renczesstefan
Repo: netgrif/application-engine PR: 339
File: application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticTaskQueueManager.java:38-45
Timestamp: 2025-08-20T07:27:02.660Z
Learning: When reviewing ElasticTaskQueueManager changes, task.getTask().getId() returns the document identifier while task.getTaskId() returns the business task identifier. The queue operations should use consistent identifiers throughout the lifecycle (scheduling, processing, cleanup).
Applied to files:
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticQueueManager.java
⏰ 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). (6)
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: Build
- GitHub Check: task-list-completed
🔇 Additional comments (1)
application-engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticQueueManager.java (1)
141-141: Verify recursion depth is acceptable for production configurations.The
flush()→checkQueue()→flush()pattern (lines 141, 184) creates recursion with depth proportional to⌈queue.size() / maxBatchSize⌉. With extreme configurations (e.g.,maxBatchSize=10and a queue approachingmaxQueueSize=100,000), this could result in ~10,000 nested calls and risk stack overflow.While this was previously discussed and marked as addressed, please confirm that production configurations maintain reasonable ratios (e.g.,
maxBatchSize ≥ 100) to keep recursion depth manageable (≤ 100-200 levels).Alternatively, if you want to eliminate the risk entirely, consider converting to an iterative loop:
private synchronized void flush() { - if (queue.isEmpty()) { - return; - } - - List<BulkOperationWrapper> batch = new ArrayList<>(); - while (!queue.isEmpty() && batch.size() < queueProperties.getMaxBatchSize()) { - batch.add(queue.poll()); - } - - String uuid = UUID.randomUUID().toString(); - try { - log.debug("Index started with batch size: {} and id: {}", batch.size(), uuid); - elasticsearchClient.bulk(new BulkRequest.Builder().operations(batch.stream().map(BulkOperationWrapper::getOperation).toList()).refresh(queueProperties.getRefreshPolicy()).build()); - log.debug("Index finished with batch size: {} and id: {}", batch.size(), uuid); - checkQueue(); - } catch (Exception e) { - log.error("Bulk operation failed for batch id: {} with {} operations. " + - "Operations will be retried via scheduled indexing or manual reindex.", - uuid, batch.size(), e); - return; - } - try { - publishEventsOfBatch(batch); - } catch (Exception e) { - log.error("Event publishing failed for batch id: {}", uuid, e); - } + while (!queue.isEmpty()) { + List<BulkOperationWrapper> batch = new ArrayList<>(); + while (!queue.isEmpty() && batch.size() < queueProperties.getMaxBatchSize()) { + batch.add(queue.poll()); + } + + String uuid = UUID.randomUUID().toString(); + try { + log.debug("Index started with batch size: {} and id: {}", batch.size(), uuid); + elasticsearchClient.bulk(new BulkRequest.Builder() + .operations(batch.stream().map(BulkOperationWrapper::getOperation).toList()) + .refresh(queueProperties.getRefreshPolicy()) + .build()); + log.debug("Index finished with batch size: {} and id: {}", batch.size(), uuid); + } catch (Exception e) { + log.error("Bulk operation failed for batch id: {} with {} operations. " + + "Operations will be retried via scheduled indexing or manual reindex.", + uuid, batch.size(), e); + resetTimer(); + return; + } + try { + publishEventsOfBatch(batch); + } catch (Exception e) { + log.error("Event publishing failed for batch id: {}", uuid, e); + } + + // Stop aggressive draining once queue drops below threshold + if (queue.size() < queueProperties.getMaxBatchSize()) { + resetTimer(); + return; + } + } }Then simplify
checkQueue()to just callflush()orresetTimer():private void checkQueue() { if (queue.size() >= queueProperties.getMaxBatchSize()) { flush(); } else { resetTimer(); } }Also applies to: 182-188
...engine/src/main/java/com/netgrif/application/engine/elastic/service/ElasticQueueManager.java
Show resolved
Hide resolved
Retoocs
previously approved these changes
Nov 10, 2025
tuplle
previously approved these changes
Nov 10, 2025
renczesstefan
dismissed stale reviews from coderabbitai[bot], tuplle, and Retoocs
The merge-base changed after approval.
18 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Refactored case indexing/deleting to use bulk index/delete, and avoid to use elasticsearch spring repository.
Implements NAE-2250
Dependencies
No new dependencies were introduced.
Third party dependencies
No new dependencies were introduced.
Blocking Pull requests
There are no dependencies on other PR.
How Has Been This Tested?
This was tested manually and with unit tests.
Test Configuration
Checklist:
Summary by CodeRabbit
New Features
Refactor
Chores
Bug Fixes
Breaking Change