Repository navigation
CBG-5900: Add support to compact channel history with Channel Names - #8857
RIT3shSapata wants to merge 2 commits into
Conversation
- compact channel history with channel names - parameterize the existing tests to test for channel name based compaction - update the REST API Endpoint - update the API spec - update inline comments and documentation
Redocly previews |
RIT3shSapata
changed the title
|
Droid finished @RIT3shSapata's task —— View job Review summaryThis PR's overall approach (mutual exclusivity between seq and channels, plus tests and OpenAPI updates) looks solid. The main correctness edge case is that compaction can currently delete Channels-map entries with Seq==0, even though those entries are treated as current memberships by getCurrentChannels. |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Channel matching introduces quadratic processing for large documents and channel lists.
Review effort: Balanced
Findings: 1
Open (1)
What changed in this PR
Adds channel-name-based document channel-history compaction alongside sequence-based compaction.
Changes:
- Adds mutually exclusive
seqandchannelsrequest modes. - Implements channel-name filtering while preserving active memberships.
- Updates API documentation and parameterizes compaction tests.
| File | Description |
|---|---|
db/crud.go |
Implements channel-based compaction. |
rest/doc_api.go |
Validates and handles the new request mode. |
rest/api_test.go |
Tests both compaction modes. |
rest/doc_api_test.go |
Tests request validation. |
docs/api/paths/admin/keyspace-_channel_history-compact.yaml |
Documents the new API option. |
db/import_test.go |
Updates the compaction call signature. |
db/hybrid_logical_vector_test.go |
Updates the compaction call signature. |
db/attachment_test.go |
Updates the compaction call signature. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| var del bool | ||
| if seq != 0 { | ||
| del = channel.End <= seq | ||
| } else if len(channels) > 0 { | ||
| del = slices.Contains(channels, channel.Name) |
There was a problem hiding this comment.
I think this comment is valid, particularly given some of the known use cases related to large numbers of channels per doc - the iteration over channels to create the map wouldn't be significantly more expensive than a single slices.Contains, and you'd only need to do it once.
![factory-droid[bot]](https://avatars.githubusercontent.com/in/358017?s=60&v=4){kind=small}
| return del | ||
| }) | ||
|
|
||
| for chanName, chanEntry := range doc.SyncData.Channels { |
There was a problem hiding this comment.
[P1] Avoid compacting Channels entries with Seq==0
SyncData.getCurrentChannels treats both nil removals and non-nil removals with Seq==0 as current channel memberships. CompactDocChannelHistory currently deletes non-nil entries whenever chanEntry.Seq <= seq (seq mode) or when the channel name is listed (channels mode), which can drop a channel that is still considered current and change the effective channel set.
Consider skipping deletions for chanEntry.Seq==0 (consistent with GetDocChannelHistory, which only reports removals where Seq != 0), for example:
| for chanName, chanEntry := range doc.SyncData.Channels { | |
| var del bool | |
| if seq != 0 { | |
| del = chanEntry.Seq != 0 && chanEntry.Seq <= seq | |
| } else if len(channels) > 0 { | |
| del = chanEntry.Seq != 0 && slices.Contains(channels, chanName) | |
| } |
| var del bool | ||
| if seq != 0 { | ||
| del = channel.End <= seq | ||
| } else if len(channels) > 0 { | ||
| del = slices.Contains(channels, channel.Name) |
There was a problem hiding this comment.
I think this comment is valid, particularly given some of the known use cases related to large numbers of channels per doc - the iteration over channels to create the map wouldn't be significantly more expensive than a single slices.Contains, and you'd only need to do it once.
| // If seq is zero, it removes the history entries of the named channels instead. | ||
| // This is used to prune stale channel assignment history to reduce storage overhead. | ||
| func (c *DatabaseCollection) CompactDocChannelHistory(ctx context.Context, docid string, seq uint64) ([]string, error) { | ||
| func (c *DatabaseCollection) CompactDocChannelHistory(ctx context.Context, docid string, seq uint64, channels []string) ([]string, error) { |
There was a problem hiding this comment.
Naming the new parameter 'channelsToDelete' might make the multiple references to channel and channels below more readable.
|
|
||
| type CompactDocChannelHistoryRequest struct { | ||
| Seq uint64 `json:"seq"` | ||
| Seq uint64 `json:"seq"` |
There was a problem hiding this comment.
I realize it was the previous handling, but why didn't we allow setting seq=0 for users that wanted to delete all channel history? They needed to use seq=1 for that?
CBG-5900
Add a
channelsoption to the document channel history compact endpoint,POST /{keyspace}/_channel_history/{docid}/compact. You can now name the channels to compact. Before, you could only give a sequence number.channelsarray toCompactDocChannelHistoryRequest. A request must set exactly one ofseqorchannels.channelsarray counts as not set.channelsparameter toCompactDocChannelHistory. Withseqset to zero, the function removes the history of the named channels.channelsmode, SG keeps the entries for channels that the document is still in.oneOfrule, and the 400 description.Pre-review checklist
base.UD(docID),base.MD(dbName))docs/apiDependencies (if applicable)
Integration Tests