feat: add connectionInfo.atlasMetadata.instanceSize COMPASS-8236 #6178
Conversation
| // TODO: I'm not sure that we should put instanceSize on here because | ||
| // right now we're only using it to calculate supportsRollingIndexes | ||
| instanceSize, | ||
| supportsRollingIndexes, |
There was a problem hiding this comment.
hmm I'm now wondering if this should even be part of atlas metadata. Maybe it goes higher up somewhere more common to a connection. Then the ui can just check if the connection can do this thing without knowing about atlas 🤔
There was a problem hiding this comment.
Good q, that would be somewhere in instance model probably? ConnectionInfo (with the exception of atlas cluster metadata) is an object that should be static cause it gets stored back to disk, probably not a great place for computed props at least as it is right now.
You make a very good point, I would consider it at the same level of "supportsTimeSeries" or other features that activates based on connection data.
Rather than adding this new pattern here it may be time we design one that we can apply consistently for other connection related feats as well.
To that point, did we consider having only the instance size here and have a useSupportsRollingIndexes(connectionInfo) (not necessarily a hook, that's just an example), or if we want to have it more generic maybe useConnectionSupports(feat, connectionInfo) helpers?
There was a problem hiding this comment.
useConnectionSupports(feat, connectionInfo)
I like the idea of having this hook / an exported util method (we already know we'll at least have global sharding to check against connection, probably more things related to the atlas cluster size and type) and only having instanceSize on the connection info itself
that would be somewhere in instance model probably?
I think domain-wise this sort of flag fits in the InstanceModel. The only reason I'd probably suggest against that is that we all know that we'd need a refactor for the instance model to bring it closer architecturally to what connections storage is doing (maybe even having them as two slices of the same store, they mostly care about the same user actions and logic flows) and so I'd just keep any new flags away from the instance as we have a good enough alternative for how to handle this right now
There was a problem hiding this comment.
ah ok, then i wonder, is this an hint we need a service / hook like the others that we have, that doesn't even take the connection info but a connection id?
There was a problem hiding this comment.
OK I've removed it and I'm putting just instanceSize here now. Then it can be checked in the generic hook useConnectionSupports() or in useSupportsRollingIndexCreation(), depending on what we decide on. I got a separate ticket for a/the hook: https://jira.mongodb.org/browse/COMPASS-8213
There was a problem hiding this comment.
As long as we don't have react components calculating these things themselves like in MMS, I'm happy ;)
There was a problem hiding this comment.
100%, this is like n. 1 thing we'd want to avoid 😄
lerouxb
changed the title
lerouxb
changed the title
| @@ -20,8 +21,10 @@ const deployment = { | |||
| ], | |||
| }; | |||
|
|
|||
| type Test = [string, ClusterDescriptionWithDataProcessingRegion, string]; | |||
There was a problem hiding this comment.
I couldn't get TypeScript to be happy with defining this as const, so I ended up just explicitly giving it a type.
| function getInstanceSize( | ||
| clusterDescription: ClusterDescription | ||
| ): string | undefined { | ||
| return getFirstInstanceSize(clusterDescription.replicationSpecList); |
There was a problem hiding this comment.
I copied this kinda literally from MMS code, but I suppose I might as well roll getInstanceSize in here.
This just puts instanceSize on connectionInfo.atlasMetadata. Will add a hook that uses it in https://jira.mongodb.org/browse/COMPASS-8213