ISSUE-4234 Add pagination to Get Tenants endpoint #5027

DataDavD · 2024-05-27T23:19:34Z

Updated GetConsistentTenants endpoint with after and limit vars in order to implement pagination for th endpoint. This required refactoring the following: the implementation of QueryTenantRequest by adding the after and limit vars. Also, the implementation of getTenants (original handler.getTenant and cluster/store schema .getTenants) method with the addition of after and limit vars which included the addition of implementation of pagination in its code definition.

Note, we created priority queue package to represent a max-heap of string items. This is used to implement pagination.

after is the tenant for which GET of tenants will be after. limit is the limit of tenants we GET.

Added test for GetConsistenTenants endpoint.

Added additional test case for QueryTenants test case in cluster store test for TestServiceEndpoints to ensure after and limit worked correctly with the cluster store as well.

nathanwilk7

This is looking really good and coming along nicely, thanks for doing this work! I know this PR might not be ready for an "official" review yet, but I wanted to leave some comments now with the hope that they'll be helpful. Many of these are "nit's" which you're welcome to do or ignore, however some of the comments are important. Happy to discuss anything in more detail of course

nathanwilk7 · 2024-05-30T22:38:52Z

adapters/handlers/rest/handlers_schema.go

@@ -274,7 +274,7 @@ func (s *schemaHandlers) deleteTenants(params schema.TenantsDeleteParams,
 func (s *schemaHandlers) getTenants(params schema.TenantsGetParams,
 	principal *models.Principal,
 ) middleware.Responder {
-	tenants, err := s.manager.GetConsistentTenants(params.HTTPRequest.Context(), principal, params.ClassName, *params.Consistency, nil)
+	tenants, err := s.manager.GetConsistentTenants(params.HTTPRequest.Context(), principal, params.ClassName, *params.Consistency, nil, nil, nil)


Suggested change

tenants, err := s.manager.GetConsistentTenants(params.HTTPRequest.Context(), principal, params.ClassName, *params.Consistency, nil, nil, nil)

tenants, err := s.manager.GetConsistentTenants(params.HTTPRequest.Context(), principal, params.ClassName, *params.Consistency, nil, params.After, params.Limit)

nathanwilk7 · 2024-05-30T23:23:40Z

cluster/utils/priorityqueue/queue.go

+	more  func(items []string, i, j int) bool
+}
+
+// NewMaxStringPriorityQueue constructs a priority queue with more.


Suggested change

// NewMaxStringPriorityQueue constructs a priority queue with more.

// NewMaxStringPriorityQueue constructs a priority queue with the specified initial capacity (initial length is always 0).

nathanwilk7 · 2024-05-30T23:28:01Z

cluster/store/schema.go

+	// TODO only one of these checks?
+	for pq.Len() > 0 && resIndex < len(res) {


nit

Suggested change

// TODO only one of these checks?

for pq.Len() > 0 && resIndex < len(res) {

for pq.Len() > 0 {

nathanwilk7 · 2024-05-30T23:29:47Z

cluster/store/schema.go

+	for i, j := 0, len(res)-1; i < j; i, j = i+1, j-1 {
+		res[i], res[j] = res[j], res[i]
+	}


nit

Suggested change

for i, j := 0, len(res)-1; i < j; i, j = i+1, j-1 {

res[i], res[j] = res[j], res[i]

}

slices.Reverse(res)

nathanwilk7 · 2024-05-30T23:41:42Z

cluster/store/schema.go

+	return res
+}
+
+func (s *schema) getTenants(class string, tenants []string, after *string, limit *int64) ([]*models.Tenant, error) {


nit, I'm concerned that having after and limit as pointers here makes the code below less legible and increase the potential for future changes to inadvertently introduce nil pointer issues. I'd love to hear your thoughts on whether you think it would be better to convert the pointers to basic types (eg use default values if nil) before calling this function or not? Not sure exactly how "high" up the stack we should push this conversion if we want to avoid passing pointers down this far.

let me think about this a bit more, but to clarify you are just asking if I think it it could be better to handle the nil values of these pointers (and using default/zero value if they are nil) up the stack?

hey @nathanwilk7 just curious if you saw my clarifying question above.

nathanwilk7 · 2024-05-31T00:43:49Z

usecases/schema/fakes_test.go

+	if args.Get(0) == nil {
+		return nil, 0, args.Error(2)
+	}
+	return args.Get(0).([]*models.Tenant), args.Get(1).(uint64), args.Error(2)


nit

Suggested change

if args.Get(0) == nil {

return nil, 0, args.Error(2)

}

return args.Get(0).([]*models.Tenant), args.Get(1).(uint64), args.Error(2)

tenant := args.Get(0)

if tenant == nil {

return nil, 0, args.Error(2)

}

shardVersion := args.Get(1)

return tenant.([]*models.Tenant), shardVersion.(uint64), args.Error(2)

nathanwilk7 · 2024-05-31T01:22:20Z

usecases/schema/tenant.go

@@ -169,45 +172,60 @@ func (h *Handler) DeleteTenants(ctx context.Context, principal *models.Principal
 // GetTenants is used to get tenants of a class.
 //
 // Class must exist and has partitioning enabled
-func (h *Handler) GetTenants(ctx context.Context, principal *models.Principal, class string) ([]*models.Tenant, error) {
+func (h *Handler) GetTenants(ctx context.Context, principal *models.Principal, class string,


I believe this func is currently unused can you confirm that is correct on both this branch and main and if so, delete it and delete the getTenants function in this same file below?

nathanwilk7 · 2024-05-31T01:26:01Z

usecases/schema/tenant.go

-			ts = make([]*models.Tenant, n)
-		} else if n < len(ts) {
-			ts = ts[:n]
+		if after != nil && limit != nil {


This code is similar to the cluster/store/schema.go:getTenants, can it be changed to avoid duplicating the logic (eg "DRY'd up"?)

nathanwilk7 · 2024-05-31T01:26:53Z

usecases/schema/tenant.go

@@ -260,24 +280,72 @@ func IsLocalActiveTenant(phys *sharding.Physical, localNode string) bool {
 		phys.Status == models.TenantActivityStatusHOT
 }

-func (h *Handler) getTenantsByNames(class string, names []string) ([]*models.Tenant, error) {
+func (h *Handler) getTenantsByNames(class string, names []string, after *string, limit *int64) ([]*models.Tenant, error) {


Reminder we may want to revisit this once we've decided on how the API should work for all parameter combintations

nathanwilk7 · 2024-05-31T01:28:59Z

usecases/schema/tenant.go

+	}
+}
+
+func getAllTenantsPriorityQueue(shards map[string]sharding.Physical, after string, limit int) []*models.Tenant {


This looks to me like essentially (exactly?) the same function as the one in cluster/store/schema.go, can we avoid duplicating this logic?

nathanwilk7 · 2024-05-31T01:33:21Z

I also just noticed that while I was reviewing you pushed some changes (crazy timing!). If any of my comments don't apply after the recent commit, just let me know

DataDavD · 2024-06-02T15:59:50Z

Hey @nathanwilk7 sorry for the late reply. Been super busy and only just now saw your comments. I'll try and get to at least some of them today or tomorrow at the latest, so we can push this PR forward.

Thank you for helping out!!!

cheers
david

sonarcloud · 2024-06-04T08:43:37Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

DataDavD · 2024-06-04T09:10:36Z

hey @nathanwilk7 FYI, I made a handful of changes. As you noted before, we should get the expected API behavior hashed, which will also help me adjust some of my now failing tests given the existing random order if after is not used, for example.

DataDavD · 2024-06-04T09:00:40Z

adapters/handlers/rest/handlers_schema.go

@@ -292,7 +292,7 @@ func (s *schemaHandlers) getTenants(params schema.TenantsGetParams,
 }

 func (s *schemaHandlers) tenantExists(params schema.TenantExistsParams, principal *models.Principal) middleware.Responder {
-	if err := s.manager.ConsistentTenantExists(params.HTTPRequest.Context(), principal, params.ClassName, *params.Consistency, params.TenantName); err != nil {
+	if err := s.manager.ConsistentTenantExists(params.HTTPRequest.Context(), principal, params.ClassName, *params.Consistency, params.TenantName, nil, nil); err != nil {


@nathanwilk7 this should probably change as well. Currently it's using the same function just see if a tenant exists; it gets the full list of tenants and then checks. That's why I just left after and limit nil here since they are technically not needed. However, that's probably a sign this function needs to change. Should I do that as part of this PR or push it to another PR after to handle after this one?

DataDavD · 2024-06-04T09:02:26Z

cluster/store/schema.go

+	return res
+}
+
+func (s *schema) getTenants(class string, tenants []string, after *string, limit *int64) ([]*models.Tenant, error) {


let me think about this a bit more, but to clarify you are just asking if I think it it could be better to handle the nil values of these pointers (and using default/zero value if they are nil) up the stack?

DataDavD · 2024-06-04T09:03:23Z

cluster/store/schema.go

 		return nil
 	}
 	return res, meta.RLockGuard(f)
 }

+func CappedTenants(limit *int64, res []*models.Tenant, tenants []string, ss *sharding.State) []*models.Tenant {


This name is terrible and needs to probably be changed, but it's late and I drew a blank lol. Help with variable names always accepted 🫶

DataDavD · 2024-06-11T18:59:47Z

cluster/store/schema.go

+	return res
+}
+
+func (s *schema) getTenants(class string, tenants []string, after *string, limit *int64) ([]*models.Tenant, error) {


hey @nathanwilk7 just curious if you saw my clarifying question above.

DataDavD · 2024-06-11T19:00:55Z

cluster/store/schema.go

+		}
+
+		if limit != nil {
+			res = make([]*models.Tenant, 0, *limit)


Hey @nathanwilk7 and @tsmith023 sorry for the bother, but just curious if Tommy saw Nathan and I's comments. I wanted to avoid proceeding too much since I don't think it's my place to make API design decisions at this point.

Updated GetConsistentTenants endpoint with `after` and `limit` vars in order to implement pagination for th endpoint. This required refactoring the following: the implementation of QueryTenantRequest by adding the `after` and `limit` vars. Also, the implementation of getTenants (original handler.getTenant and cluster/store schema .getTenants) method with the addition of `after` and `limit` vars which included the addition of implementation of pagination in its code definition. Note, we created priority queue package to represent a max-heap of string items. This is used to implement pagination. `after` is the tenant for which GET of tenants will be AFTER. `limit` is the limit of tenants we GET. Added test for GetConsistenTenants endpoint. Added additional test case for QueryTenants test case in cluster store test for TestServiceEndpoints to ensure after and limit worked correctly with the cluster store as well.

DataDavD force-pushed the ddansby/ISSUE-4234/add-pagination-to-get-tenants-endpoint branch 3 times, most recently from 2d3bc9e to 3555249 Compare May 28, 2024 06:36

nathanwilk7 self-requested a review May 30, 2024 21:49

DataDavD force-pushed the ddansby/ISSUE-4234/add-pagination-to-get-tenants-endpoint branch from 3555249 to 3d0cbba Compare May 31, 2024 01:19

nathanwilk7 requested changes May 31, 2024

View reviewed changes

DataDavD force-pushed the ddansby/ISSUE-4234/add-pagination-to-get-tenants-endpoint branch 2 times, most recently from dfc33c7 to 5898f5f Compare June 4, 2024 08:42

DataDavD commented Jun 19, 2024

View reviewed changes

DataDavD force-pushed the ddansby/ISSUE-4234/add-pagination-to-get-tenants-endpoint branch from 5898f5f to 6856272 Compare June 20, 2024 08:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ISSUE-4234 Add pagination to Get Tenants endpoint #5027

ISSUE-4234 Add pagination to Get Tenants endpoint #5027

DataDavD commented May 27, 2024 •

edited

Loading

nathanwilk7 left a comment

nathanwilk7 May 30, 2024

nathanwilk7 May 30, 2024

nathanwilk7 May 30, 2024

nathanwilk7 May 30, 2024

nathanwilk7 May 30, 2024

DataDavD Jun 4, 2024

DataDavD Jun 11, 2024

nathanwilk7 May 31, 2024

nathanwilk7 May 31, 2024

nathanwilk7 May 31, 2024

nathanwilk7 May 31, 2024

nathanwilk7 May 31, 2024

nathanwilk7 commented May 31, 2024

DataDavD commented Jun 2, 2024

sonarcloud bot commented Jun 4, 2024

DataDavD commented Jun 4, 2024

DataDavD Jun 4, 2024

DataDavD Jun 4, 2024

DataDavD Jun 4, 2024

DataDavD Jun 11, 2024

DataDavD Jun 11, 2024

	tenants, err := s.manager.GetConsistentTenants(params.HTTPRequest.Context(), principal, params.ClassName, *params.Consistency, nil, nil, nil)
	tenants, err := s.manager.GetConsistentTenants(params.HTTPRequest.Context(), principal, params.ClassName, *params.Consistency, nil, params.After, params.Limit)

	// NewMaxStringPriorityQueue constructs a priority queue with more.
	// NewMaxStringPriorityQueue constructs a priority queue with the specified initial capacity (initial length is always 0).

		// TODO only one of these checks?
		for pq.Len() > 0 && resIndex < len(res) {

	// TODO only one of these checks?
	for pq.Len() > 0 && resIndex < len(res) {
	for pq.Len() > 0 {

ISSUE-4234 Add pagination to Get Tenants endpoint #5027

Are you sure you want to change the base?

ISSUE-4234 Add pagination to Get Tenants endpoint #5027

Conversation

DataDavD commented May 27, 2024 • edited Loading

nathanwilk7 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nathanwilk7 commented May 31, 2024

DataDavD commented Jun 2, 2024

sonarcloud bot commented Jun 4, 2024

Quality Gate passed

DataDavD commented Jun 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DataDavD commented May 27, 2024 •

edited

Loading