Feature/cms backend #776
Feature/cms backend #776
Conversation
|
|
||
| public async Task HandleCreateCentralManagementServerRequest(CreateCentralManagementServerParams createCmsParams, RequestContext<ListRegisteredServersResult> requestContext) | ||
| { | ||
| Logger.Write(TraceEventType.Verbose, "HandleCreateCentralManagementServerRequest"); |
There was a problem hiding this comment.
I noticed none of the handlers are async. If there are any long running operations we should make sure they are async so we don't block the ipc handler.
There was a problem hiding this comment.
Sure will do for new connections. I noticed we do async notification for new connection but not for list databases like scenarios - so will follow the same.
There was a problem hiding this comment.
I wouldn't recommend following the connection service example. The notification system it uses is outdated and complicated.
There was a problem hiding this comment.
| { | ||
| public string RegisteredServerName { get; set; } | ||
|
|
||
| public string RegisterdServerDescription { get; set; } |
There was a problem hiding this comment.
small typo: RegisteredServerDescription
| ServerConnection conn = await ValidateAndCreateConnection(createCmsParams.ConnectParams); | ||
|
|
||
| // Get Current Reg Servers on CMS | ||
| RegisteredServersStore store = new RegisteredServersStore(conn); |
There was a problem hiding this comment.
TODO: Using the alias/display name passed for CMS
| namespace Microsoft.SqlTools.ServiceLayer.Cms | ||
| { | ||
| /// <summary> | ||
| /// Main class for DacFx service |
|
|
||
| // Get Current Reg Servers on CMS | ||
| RegisteredServersStore store = new RegisteredServersStore(conn); | ||
| ServerGroup parentGroup = store.DatabaseEngineServerGroup; //TODO Do we need other types like Integrated Service and Analysis Service |
There was a problem hiding this comment.
we're not supporting SSIS or AS in this codebase, so probably don't need the TODO. If we want that in the future there will need to be large scale refactoring in a bunch of places.
| } | ||
| catch (Exception e) | ||
| { | ||
| // Exception related to run tak will be captured here |
| /// <summary> | ||
| /// Internal variable for testability | ||
| /// </summary> | ||
| internal Task CmsTask { get; set; } |
There was a problem hiding this comment.
what are you using this property for? I don't see it being accessed outside this class, and then only to assign to it.
There was a problem hiding this comment.
This is only used in tests. Since the call to Handle functions in Service is async, we need a way to wait for task to complete to validate requestContext result. This was one pattern used for testing across multiple services so used the same.
There was a problem hiding this comment.
makes sense. For some reason when I search for usages yesterday I didn't see any, but now I see this being used through out the tests. Maybe I had a typo in search box 😄
| { | ||
| if (registerdServerStore == null) | ||
| { | ||
| registerdServerStore = RegisteredServersStore.LocalFileStore; |
There was a problem hiding this comment.
should you assign using the setter (i.e. this.ServerStore = …) to go through the lock, or take the lock explicitly here?
| } | ||
|
|
||
| /// <summary> | ||
| /// Parmaters to Add Registered Server to top level CMS |
There was a problem hiding this comment.
..note this same typo appears to have been copy\pasted a few times
|
|
||
| public string RegisteredServerDescription { get; set; } | ||
|
|
||
| public ConnectionDetails RegServerConnectionDetails { get; set; } |
There was a problem hiding this comment.
we should be consistent whether to use RegisteredServer or RegServer, personally I would always use RegisteredServer event though it's too verbose.
| RequestType<CreateCentralManagementServerParams, ListRegisteredServersResult>.Create("cms/createCms"); | ||
| } | ||
|
|
||
| public class ListRegisteredServerRequest |
There was a problem hiding this comment.
should this be ListRegisteredServersRequest (plural)?
| public class ListRegisteredServerRequest | ||
| { | ||
| public static readonly RequestType<ListRegisteredServerParams, ListRegisteredServersResult> Type = | ||
| RequestType<ListRegisteredServerParams, ListRegisteredServersResult>.Create("cms/listRegServers"); |
There was a problem hiding this comment.
I'd write out RegisteredServers completely in the JSON-RPC method names.
|
|
||
| public string RelativePath { get; set; } | ||
|
|
||
| public ConnectionDetails connectionDetails { get; set; } |
There was a problem hiding this comment.
connectionDetails should be ConnectionDetails
Cms SQL Tools Service first set of Apis