-
Notifications
You must be signed in to change notification settings - Fork 1
updated HasResolveCustomSource PR for latest branch #7
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.
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
base: metadata-separation-multi-source
Are you sure you want to change the base?
updated HasResolveCustomSource PR for latest branch #7
Conversation
|
|
||
| schemaCacheE <- runExceptT | ||
| $ peelMetadataRun (RunCtx adminUserInfo httpManager sqlGenCtx) metadata | ||
| $ peelMetadataRun (RunCtx adminUserInfo httpManager sqlGenCtx {- TODO: -} defaultResolveCustomSource) metadata |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here, we initialize and build the RebuildableSchemaCache for the OSS server. No other customization exist (via type classes). We can use default here for OSS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this also where we build the cache for Pro then? So wouldn't we need to add the type class here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or do you mean it's just the connection used to read the Postgres metadata? I guess what I'm asking is - will the ResolvedSources be stored in the schema cache?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this also where we build the cache for Pro then?
I'm not sure about this. If yes, then we should add the type class here too.
| runAsAdmin sqlGenCtx httpManager metadata m = | ||
| fmap (fmap fst) $ runExceptT $ | ||
| peelMetadataRun (RunCtx adminUserInfo httpManager sqlGenCtx) metadata m | ||
| peelMetadataRun (RunCtx adminUserInfo httpManager sqlGenCtx {- TODO: -} defaultResolveCustomSource) metadata m |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here too we can use the default one.
| Just jVal -> decodeValue jVal | ||
| Nothing -> throw400 InvalidJSON "invalid json" | ||
| let runCtx = RunCtx adminUserInfo httpManager (SQLGenCtx False) | ||
| let runCtx = RunCtx adminUserInfo httpManager (SQLGenCtx False) {- TODO: -} defaultResolveCustomSource |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here too we can use the default one.
| let tableMetadatas = mapFromL _tmTable $ HM.elems postRelMap | ||
| sources = HM.singleton defaultSource $ | ||
| SourceMetadata defaultSource tableMetadatas functions defaultSourceConfig | ||
| SourceMetadata defaultSource tableMetadatas functions defaultSourceConfig [] {- TODO: not sure what to do here -} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do think there's no notion of read_replicas in the OSS server before. This line of code is trying to migrate the metadata of the older single source database to the newer multi source support. Hence we can safely use [] which specifies there's no read_replica configuration.
|
I'm going to wait until the monorepo merge is done, and then pick this back up. Hopefully it should be easier to collaborate on a single shared branch at that point. cc @ecthiender |
@rakeshkky Please let me know what you think. There are a few TODO items that I'm not sure about.