[ENH] GetCollectionSize on SysDB read replica #3503
Conversation
Reviewer ChecklistPlease leverage this checklist to ensure your code review is thorough before approving Testing, Bugs, Errors, Logs, Documentation
System Compatibility
Quality
|
| @@ -33,7 +32,7 @@ type Coordinator struct { | |||
| deleteMode DeleteMode | |||
| } | |||
|
|
|||
| func NewCoordinator(ctx context.Context, db *gorm.DB, deleteMode DeleteMode) (*Coordinator, error) { | |||
There was a problem hiding this comment.
A bit of clean up—we don't actually use the db in the coordinator since we access the globalDB.
| @@ -142,6 +143,31 @@ func (s *collectionDb) getCollections(id *string, name *string, tenantID string, | |||
| return | |||
| } | |||
|
|
|||
| func (s *collectionDb) GetCollectionSize(id string) (uint64, error) { | |||
| query := s.read_db.Table("collections"). | |||
There was a problem hiding this comment.
Note the use of read_db here.
drewkim
force-pushed
the
drew/_enh_getcollectionsize_on_sysdb_read_replica
branch
from
0e9a6c3 to
d0e4a2f
Compare
| @@ -25,6 +25,7 @@ import ( | |||
| type APIsTestSuite struct { | |||
| suite.Suite | |||
| db *gorm.DB | |||
| read_db *gorm.DB | |||
There was a problem hiding this comment.
do you think it's worth using gorm's DBResolver or is that too much framework overhead?
There was a problem hiding this comment.
We don't want to cutover all of our reads to the read replica, which it seems DBResolver does automatically. Probably not worth at this point
There was a problem hiding this comment.
it looked like you could configure it to not use the replica by default and then explicitly use the replica with
tx := db.Clauses(dbresolver.Use("secondary"), dbresolver.Write).Begin()but not strongly advocating for it
| @@ -384,4 +392,5 @@ service SysDB { | |||
| rpc FlushCollectionCompaction(FlushCollectionCompactionRequest) returns (FlushCollectionCompactionResponse) {} | |||
| rpc RestoreCollection(RestoreCollectionRequest) returns (RestoreCollectionResponse) {} | |||
| rpc ListCollectionVersions(ListCollectionVersionsRequest) returns (ListCollectionVersionsResponse) {} | |||
| rpc GetCollectionSize(GetCollectionSizeRequest) returns (GetCollectionSizeResponse) {} | |||
There was a problem hiding this comment.
what do you think about calling this
| rpc GetCollectionSize(GetCollectionSizeRequest) returns (GetCollectionSizeResponse) {} | |
| rpc GetApproximateCollectionSize(GetCollectionSizeRequest) returns (GetCollectionSizeResponse) {} |
since this is reading from the replica and it may lag behind the leader?
| @@ -142,6 +143,29 @@ func (s *collectionDb) getCollections(id *string, name *string, tenantID string, | |||
| return | |||
| } | |||
|
|
|||
| func (s *collectionDb) GetCollectionSize(id string) (uint64, error) { | |||
There was a problem hiding this comment.
Is there some magical reason why some of these methods take a *string versus ones like this that take a string? I don't have enough context on why one would make sense over the other but it would be nice to be consistent. I feel like string should be the way to go here, but the callsite for this is using an interface which requires a deref, which is not great.
There was a problem hiding this comment.
Updated the call site to not require a deref. I think the use of *string in some places is because it isn't a required param.
| defer span.End() | ||
| } | ||
|
|
||
| total_records_post_compaction, err := tc.metaDomain.CollectionDb(ctx).GetCollectionSize(*types.FromUniqueID(collectionID)) |
There was a problem hiding this comment.
The *types.FromUniqueID(collectionID) part is a little strange. I guess it's fine if we're already doing it all over, but technically this could blow up if this returned nil.
There was a problem hiding this comment.
Updated to use .String() instead of a deref
There was a problem hiding this comment.
I don't have anything blocking, but would be good to get @sanketkedia and @rohitcpbot to have a look before approving.
Description of changes
Summarize the changes made by this PR.
SysDBGo coordinator to a DB read replicaGetCollectionSize, to return the total records in a collection, pulling from the reader DBTest plan
How are these changes tested?
pytestfor python,yarn testfor js,cargo testfor rustDocumentation Changes
Are all docstrings for user-facing APIs updated if required? Do we need to make documentation changes in the docs repository?