Conversation
There was a problem hiding this comment.
Pull request overview
Adds additional DCP resync status metrics (targeted document estimate + errored document count) and introduces a collection-level document count query to support those stats.
Changes:
- Add a
CountAllDocshelper (view + N1QL) and a new N1QLCOUNT(*)query type/stat (QueryTypeCountDocs). - Track and expose new resync stats (
docs_targeted,docs_error_count) in DCP resync status, including persistence/resume support. - Extend resync tests to validate
DocsTargetedbehavior; add unit test forCountAllDocs.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| db/query.go | Adds QueryCountDocs and CountAllDocs to count documents via view/N1QL. |
| db/query_test.go | Adds unit test coverage for CountAllDocs across delete/purge cases. |
| db/database.go | Registers the new query type for query stats initialization. |
| db/database_collection.go | Introduces DatabaseCollections helper type for resync collection handling. |
| db/background_mgr_resync_dcp.go | Adds targeted/errored resync stats and collection selection refactor. |
| db/background_mgr_resync_dcp_test.go | Extends resync init/start/resume tests to assert DocsTargeted. |
Comments suppressed due to low confidence (2)
db/background_mgr_resync_dcp.go:521
- This change adds new fields to the resync status JSON (
docs_error_countanddocs_targeted) returned by GET /{db}/_resync. The OpenAPI schemaResync-statusindocs/api/components/schemas.yamlcurrently documents onlydocs_changedanddocs_processed, so the API specs should be updated to include these new properties (and required-ness if applicable).
DocsErrored int64 `json:"docs_error_count"`
DocsTargeted uint64 `json:"docs_targeted"`
}
// SetProcessStatus reports the new status that was serialized to the bucket along with the last status that polled
// using GetProcessStatus.
func (r *ResyncManagerDCP) SetProcessStatus(ctx context.Context, previousStatus []byte, newStatus []byte) {
db/background_mgr_resync_dcp.go:254
- New
docs_error_count/DocsErroredbehavior is introduced (local counter increment + surfaced in status), but there doesn’t appear to be any unit/integration test asserting it increments on a resync document failure and is reported via GET /{db}/_resync. Adding a focused test that forces a resync update error and checksDocsErroredwould help prevent regressions.
} else if err != base.ErrUpdateCancel {
r.docsErroredLocal.Add(1)
base.WarnfCtx(ctx, "Resync: Error updating doc %q: %v", base.UD(docID), err)
return false
}
return true
}
if r.hasAllCollections {
| // DatabaseCollections is a set of collections | ||
| type DatabaseCollections []*DatabaseCollection | ||
|
|
||
| // getIDs returns the collection IDs for each collection | ||
| func (c *DatabaseCollections) getIDs() []uint32 { | ||
| ids := make([]uint32, 0, len(*c)) | ||
| for _, collection := range *c { | ||
| ids = append(ids, collection.GetCollectionID()) | ||
| } | ||
| return ids | ||
| } | ||
|
|
||
| // getNames return the names of the collections | ||
| func (c *DatabaseCollections) getNames() base.CollectionNames { | ||
| names := base.NewCollectionNames() | ||
| for _, collection := range *c { | ||
| names.Add(collection.dataStore) | ||
| } | ||
| return names |
Redocly previews |
adamcfraser
left a comment
adamcfraser
left a comment
There was a problem hiding this comment.
General approach looks good, some questions on the details.
|
|
||
| // View Query | ||
| if c.useViews() { | ||
| opts := Body{"stale": false, "reduce": true} |
There was a problem hiding this comment.
Since we're not relying on this for anything other than an estimate, can we use "stale":ok for improved view performance?
| countDocsQueryStatement := replaceSyncTokensQuery(QueryCountDocs.statement, c.UseXattrs()) | ||
| countDocsQueryStatement = replaceIndexTokensQuery(countDocsQueryStatement, sgIndexes[IndexAllDocs], c.UseXattrs(), c.numIndexPartitions()) | ||
|
|
||
| results, err := N1QLQueryWithStats(ctx, c.dataStore, QueryTypeCountDocs, countDocsQueryStatement, nil, base.RequestPlus, QueryCountDocs.adhoc, c.dbStats(), c.slowQueryWarningThreshold()) |
There was a problem hiding this comment.
Similar to the above - should we use base.NotBounded instead of base.RequestPlus since we're not requiring that level of consistency for the results?
There was a problem hiding this comment.
If I use NotBounded here, then this value is wrong when running the integration tests. I'm inclined to change this back to RequestPlus because it largely doesn't matter and then we'd know if the query goes wrong.
This seems to be a problem in a few different ways.
- Add 5 docs, sometimes not all 5 show up
- Other tests from the bucket pool actually have old documents/tombstones that are found. These are purged but still show up in the index.
| var row struct { | ||
| Count uint64 `json:"count"` | ||
| } | ||
| if results.Next(ctx, &row) { |
There was a problem hiding this comment.
Is this a good use case for results.One?
| func (c *DatabaseCollections) getNames() base.CollectionNames { | ||
| names := base.NewCollectionNames() | ||
| for _, collection := range *c { | ||
| names.Add(collection.dataStore) |
There was a problem hiding this comment.
It's a bit unexpected to have to go to the dataStore to get the scope/collection names when they are already copied to DatabaseCollection, but possibly it's more reliable in case someone doesn't use the constructor to build DatabaseCollection?
There was a problem hiding this comment.
It is just shorter for me as a programmer to use this than collection.Scope and collection.Name because Add takes a DataStore object. No particular reason.
|
|
||
| docsTargeted, err := totalResyncDocs(ctx, collections) | ||
| if err != nil { | ||
| return err |
There was a problem hiding this comment.
Wondering whether we should actually error out here, or proceed without a docsTargeted stat. Seems like it would be preferable for an unexpected error performing the count to not block resync more generally.
CBG-5379 add resync stats for targeted/error
Pre-review checklist
fmt.Print,log.Print, ...)base.UD(docID),base.MD(dbName))docs/apiIntegration Tests