feat(couchbase): add journal event persistence - #949
Conversation
dieppa
left a comment
dieppa
left a comment
There was a problem hiding this comment.
LGTM , but I would @osantana85 to take a look. He is more familiar with couchebase
There was a problem hiding this comment.
Pull request overview
Adds feature-flagged Couchbase journal event persistence with transactional audit synchronization and integration coverage.
Changes:
- Added journal mapping, constants, indexing, storage, and acknowledgement support.
- Integrated journal writes with Couchbase transactions and stage sequencing.
- Added feature-flag, rollback, and end-to-end tests.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Summary | Review notes |
|---|---|---|
utils/couchbase-util/src/main/java/io/flamingock/internal/common/couchbase/journal/JournalEventPersistenceConstants.java |
Defines journal collection defaults. | — |
utils/couchbase-util/src/main/java/io/flamingock/internal/common/couchbase/journal/JournalEventFieldConstants.java |
Defines persisted journal fields. | — |
utils/couchbase-util/src/main/java/io/flamingock/internal/common/couchbase/CouchbaseJournalEventMapper.java |
Maps journal events to Couchbase documents. | — |
utils/couchbase-util/src/main/java/io/flamingock/internal/common/couchbase/CouchbaseCollectionHelper.java |
Adds secondary-index creation support. | — |
core/target-systems/flamingock-couchbase-externalsystem-api/src/main/java/io/flamingock/externalsystem/couchbase/api/CouchbaseExternalSystem.java |
Exposes transactional Couchbase behavior. | Critical, 1 vote: New abstract API breaks existing implementations; preserve compatibility or explicitly version the breaking change. |
core/target-systems/flamingock-couchbase-externalsystem-api/build.gradle.kts |
Adds the transactional API dependency. | — |
community/flamingock-couchbase-auditstore/src/test/java/io/flamingock/store/couchbase/internal/CouchbaseAuditPersistenceJournalTest.java |
Tests transactional persistence and rollback. | — |
community/flamingock-couchbase-auditstore/src/test/java/io/flamingock/store/couchbase/CouchbaseJournalFeatureFlagE2ETest.java |
Tests feature-flagged end-to-end behavior. | — |
community/flamingock-couchbase-auditstore/src/main/java/io/flamingock/store/couchbase/internal/CouchbaseJournalEventStore.java |
Implements journal storage and acknowledgement. | Moderate, 4 votes: Acknowledgement updates should exclude already acknowledged events to avoid overcounting retries. |
community/flamingock-couchbase-auditstore/src/main/java/io/flamingock/store/couchbase/internal/CouchbaseAuditPersistence.java |
Coordinates atomic audit and journal writes. | — |
community/flamingock-couchbase-auditstore/src/main/java/io/flamingock/store/couchbase/internal/CouchbaseAuditor.java |
Supports append and current-state persistence. | — |
community/flamingock-couchbase-auditstore/src/main/java/io/flamingock/store/couchbase/CouchbaseAuditStore.java |
Wires journal-aware persistence. | Critical, 1 vote: Deprecated getPersistence() now throws unconditionally, breaking existing callers when journaling is disabled. |
Suppressed comments (1)
community/flamingock-couchbase-auditstore/src/main/java/io/flamingock/store/couchbase/internal/CouchbaseJournalEventStore.java:212
- Couchbase KV document keys are limited to 250 UTF-8 bytes, but
JournalEventpermits any non-blank stream ID and this concatenation has no size check. A sufficiently long stage/stream ID will therefore make a journal-enabled audit write fail with a key-length error and roll back its audit entry. Validate the encoded key length (with a useful configuration error) or use a bounded deterministic key representation before inserting.
private static String toKey(String streamId, long streamSequence) {
return KEY_PREFIX + "::" + streamId + "::" + streamSequence;
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| public CommunityAuditPersistence getPersistence() { | ||
| throw new UnsupportedOperationException("getPersistence shouldn't be called at Couchbase audit store; use getPersistenceFactory(stageId)"); |
| String query = String.format( | ||
| "UPDATE `%s`.`%s`.`%s` SET %s = true WHERE %s IN $eventIds RETURNING META().id", | ||
| collection.bucketName(), collection.scopeName(), collection.name(), KEY_ACKNOWLEDGED, KEY_EVENT_ID); |
| import io.flamingock.internal.common.core.transaction.TransactionalExternalSystem; | ||
|
|
||
| public interface CouchbaseExternalSystem extends ExternalSystem { | ||
| public interface CouchbaseExternalSystem extends TransactionalExternalSystem { |
There was a problem hiding this comment.
Intentional, not addressed in this PR. The move from ExternalSystem to TransactionalExternalSystem (which adds the abstract getTxWrapper()) is a deliberate, cross-target-system change: DynamoDBExternalSystem already made the identical migration in #948, which is merged. Journal-event persistence requires the audit write and the journal append to land in one transaction attempt, so a Couchbase target system must expose a transaction wrapper.
Acknowledged as a source-incompatible change for existing custom CouchbaseExternalSystem implementations; it will be called out in the release notes / version bump rather than softened with an optional default, to keep the contract consistent with the other target systems.
|
@osantana85 can you take a look, you are more. Also @davidfrigolet there are some copilot comments worth looking at. |
51326c5 to
4938e42
Compare
feat(couchbase): add journal event persistence