Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: bump protos to support list caches with metadata around limits #741

Merged
merged 7 commits into from
Aug 21, 2023
+1,253 −15,463
Merged

feat: bump protos to support list caches with metadata around limits #741

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
1ec6902
feat: bump protos to support list caches with metadata around limits
bruuuuuuuce Aug 18, 2023
7f553de
chore: package lock from public npmjs
bruuuuuuuce Aug 18, 2023
dfef03f
chore: nodejs defaults to 0
bruuuuuuuce Aug 18, 2023
5bac46a
fix: check topic/cache limits in integration tests
bruuuuuuuce Aug 18, 2023
552229f
chore: log results
bruuuuuuuce Aug 18, 2023
4e4f1ee
chore: fix int tests
bruuuuuuuce Aug 18, 2023
3e8ff02
fix: use interface instead of object
bruuuuuuuce Aug 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5,485 changes: 389 additions & 5,096 deletions packages/client-sdk-nodejs/package-lock.json
View file
Open in desktop

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion packages/client-sdk-nodejs/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@
"uuid": "8.3.2"
},
"dependencies": {
"@gomomento/generated-types": "0.69.0",
"@gomomento/generated-types": "0.73.0",
"@gomomento/sdk-core": "file:../core",
"@grpc/grpc-js": "1.9.0",
"@types/google-protobuf": "3.15.6",
Expand Down
24 changes: 21 additions & 3 deletions packages/client-sdk-nodejs/src/internal/control-client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,10 @@ import {
} from '@gomomento/sdk-core/dist/src/internal/utils';
import {normalizeSdkError} from '@gomomento/sdk-core/dist/src/errors';
import {_SigningKey} from '@gomomento/sdk-core/dist/src/messages/responses/grpc-response-types';
import {
CacheLimits,
TopicLimits,
} from '@gomomento/sdk-core/dist/src/messages/cache-info';

export interface ControlClientProps {
configuration: Configuration;
Expand Down Expand Up @@ -168,9 +172,23 @@ export class ControlClient {
if (err || !resp) {
resolve(new ListCaches.Error(cacheServiceErrorMapper(err)));
} else {
const caches = resp.cache.map(
cache => new CacheInfo(cache.cache_name)
);
const caches = resp.cache.map(cache => {
const cacheName = cache.cache_name;
const topicLimits = new TopicLimits({
maxPublishMessageSizeKb:
cache.topic_limits?.max_publish_message_size_kb || 0,
maxSubscriptionCount:
cache.topic_limits?.max_subscription_count || 0,
maxPublishRate: cache.topic_limits?.max_publish_rate || 0,
});
const cacheLimits = new CacheLimits({
maxTtlSeconds: cache.cache_limits?.max_ttl_seconds || 0,
maxItemSizeKb: cache.cache_limits?.max_item_size_kb || 0,
maxThroughputKbps: cache.cache_limits?.max_throughput_kbps || 0,
maxTrafficRate: cache.cache_limits?.max_traffic_rate || 0,
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just checking my understanding of the proto changes - cache.cache_name is always present but the cache_limits and topics_limits were modeled as optional?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

they are modeled as always present, but until the change gets all the way through the backend, they will be returned as null

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but I'm surprised the TS compiler will accept this? If the type is defined as non-null?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it just thinks that || 0 will never be true, but it doesn't fail compilation because of that

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay. interesting.

worth capturing a ticket to come back and remove the ? later?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is it different for cache_name though?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe because it is a grpc object vs a string, but I am not entirely sure

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay. it's probably fine like this. thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ill make a ticket to remove the ? for the nodejs sdk tho

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#755

return new CacheInfo(cacheName, topicLimits, cacheLimits);
});
resolve(new ListCaches.Success(caches));
}
});
Expand Down
Loading