feat(services): U2C cache logic #4593
Conversation
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
rarajes2
left a comment
rarajes2
left a comment
There was a problem hiding this comment.
- will review the UTs in the 2nd round since the diff is large.
- Let's fix the failing pipeline, add a vidcast and, fill the description with all the details
| const {services} = this.webex.internal; | ||
|
|
||
| // Wait for the postauth catalog to populate. | ||
| console.log('pkesari_canRegister invoking waitForCatalog for postauth'); |
| try { | ||
| const key = formattedQuery && Object.keys(formattedQuery || {})[0]; | ||
| if (key) { | ||
| selectionMeta = { | ||
| selectionType: key, | ||
| selectionValue: formattedQuery[key], | ||
| }; | ||
| } | ||
| } catch { | ||
| this.logger.warn('services: error building selection meta'); | ||
| } |
There was a problem hiding this comment.
see if we need try-catch for this
| let selectionMeta: {selectionType: string; selectionValue: string} | undefined; | ||
| if (serviceGroup === 'preauth' || serviceGroup === 'signin') { | ||
| try { | ||
| const key = formattedQuery && Object.keys(formattedQuery || {})[0]; |
There was a problem hiding this comment.
Are we expecting only one key at a time ?
There was a problem hiding this comment.
Yes at a time we would use either orgId or email or location to fetch the catalog
| /** | ||
| * @private | ||
| * Cache the catalog in the bounded storage. | ||
| * @param {ServiceGroup} serviceGroup - preauth, signin, postauth |
There was a problem hiding this comment.
Are we allowing this to cache all service groups ?
There was a problem hiding this comment.
Yes We are caching all service groups
| async _cacheCatalog( | ||
| serviceGroup: ServiceGroup, | ||
| hostMap: ServiceHostmap, | ||
| meta?: {selectionType: string; selectionValue: string} |
There was a problem hiding this comment.
We can create type for meta, this is repeated
| const hostMap: ServiceHostmap = | ||
| cachedGroup && cachedGroup.hostMap ? cachedGroup.hostMap : cachedGroup; | ||
| const meta: {selectionType: string; selectionValue: string} | undefined = | ||
| cachedGroup && cachedGroup.meta ? cachedGroup.meta : undefined; |
There was a problem hiding this comment.
cachedGroup && cachedGroup.meta ? cachedGroup.meta : undefined; --> cachedGroup?.meta
| if (typeof window !== 'undefined' && (window as any).localStorage) { | ||
| (window as any).localStorage.removeItem(CATALOG_CACHE_KEY_V2); | ||
| } |
There was a problem hiding this comment.
same here, having a util will simplify the logic
| this.logger.warn('services: error clearing catalog cache'); | ||
| } | ||
|
|
||
| return Promise.resolve(); |
There was a problem hiding this comment.
Should we resolve/reject based on try-catch ?
There was a problem hiding this comment.
How does rejecting it helps ? We are not going to use these methods to change any flow. All it needs to do is cache it and use the cache but any kind of faikure should not block the flows
| process.env.WEBEX_CONVERSATION_DEFAULT_CLUSTER || `${DEFAULT_CLUSTER}:${CLUSTER_SERVICE}`; | ||
| const CATALOG_CACHE_KEY_V1 = 'services.v1.u2cHostMap'; | ||
| const CATALOG_TTL_MS = 24 * 60 * 60 * 1000; // 24 hours | ||
|
|
There was a problem hiding this comment.
Please address similar comments from services-v2.ts file
2 participants
COMPLETES #CAI-6513
This pull request addresses
by making the following changes
Change Type
The following scenarios were tested
< ENUMERATE TESTS PERFORMED, WHETHER MANUAL OR AUTOMATED >
The GAI Coding Policy And Copyright Annotation Best Practices
I certified that
Make sure to have followed the contributing guidelines before submitting.