generic way to support scriptable operations #438
Conversation
llali
changed the title
| public SqlTask() | ||
| { | ||
| } | ||
|
|
There was a problem hiding this comment.
Can you create SQLtask with no params? If so, please write comment #Resolved
| { | ||
| get | ||
| { | ||
| return string.Empty; |
There was a problem hiding this comment.
what's the point of this property? #WontFix
There was a problem hiding this comment.
I added the error message to IBackupOperation so that the task manager get the error of the operation but didn't integrate the new changes with backup
In reply to: 134057582 [](ancestors = 134057582)
| { | ||
| get | ||
| { | ||
| return TargetDatabaseName; |
There was a problem hiding this comment.
why do we have a property that gets & sets another property? Should we remove this property or the other one? #WontFix
There was a problem hiding this comment.
I added the DatabaseName to the request base class assuming all requests will have a database name field. TargetDatabase is the same as DatabaseName if restore so that's why I'm implementing it this way
In reply to: 134058351 [](ancestors = 134058351)
| if(mode == TaskExecutionMode.Script || mode == TaskExecutionMode.ExecuteAndScript) | ||
| { | ||
| this.ScriptContent = GetScriptContent(); | ||
| if(SqlTask != null) |
There was a problem hiding this comment.
nit: missing space between 'f' and '(' per coding guidelines. #Resolved
assuming you take a run at fixing these.| @@ -1,6 +1,6 @@ | |||
| [](https://travis-ci.org/Microsoft/sqltoolsservice) | |||
| [](https://ci.appveyor.com/project/kburtram/sqltoolsservice) | |||
| [](https://coveralls.io/github/Microsoft/sqltoolsservice?branch=dev) | |||
| [](https://coveralls.io/github/Microsoft/sqltoolsservice?branch=master) | |||
There was a problem hiding this comment.
Nice catch :) #WontFix
| @@ -163,6 +163,16 @@ public string ScriptContent | |||
| } | |||
| } | |||
|
|
|||
| public string ErrorMessage | |||
There was a problem hiding this comment.
Minor: please add doc comments to public properties #Resolved
| metadata.Name = SR.RestoreTaskName; | ||
| metadata.IsCancelable = true; | ||
| metadata.Data = restoreDataObject; | ||
| TaskMetadata metadata = TaskMetadata.Create(restoreParams, SR.RestoreTaskName, restoreDataObject, ConnectionServiceInstance); |
There was a problem hiding this comment.
What happens to IsCancelable? Is the default True and you just override or don't? #Pending
There was a problem hiding this comment.
I removed it from here and if the cancel function is defined I assume it's cancelable
In reply to: 134042668 [](ancestors = 134042668)
| { | ||
| if (SqlTask != null) | ||
| restorePlan.PercentComplete += (object sender, PercentCompleteEventArgs e) => |
There was a problem hiding this comment.
Nice. So do we have this hooked up to the front end so you can see latest status in the task viewlet? #WontFix
There was a problem hiding this comment.
not yet, the task manager needs to show the messages
In reply to: 134043131 [](ancestors = 134043131)
| { | ||
| foreach (Restore restore in RestorePlan.RestoreOperations) | ||
| { | ||
| restore.Abort(); |
There was a problem hiding this comment.
So after everything is canceled, when do we send the overall task canceled message / exception? #WontFix
There was a problem hiding this comment.
CancelTaskAsync does that after calling this method
In reply to: 134043367 [](ancestors = 134043367)
|
|
||
| namespace Microsoft.SqlTools.ServiceLayer.TaskServices | ||
| { | ||
| public class SqlTaskOpration |
There was a problem hiding this comment.
Please add doc comments #WontFix
| @@ -3,6 +3,8 @@ | |||
| // Licensed under the MIT license. See LICENSE file in the project root for full license information. | |||
| // | |||
|
|
|||
| using System; | |||
There was a problem hiding this comment.
Is this needed? #Resolved
| /// <summary> | ||
| /// The function to run | ||
| /// </summary> | ||
| public Func<SqlTask, Task<TaskResult>> TaskToRun |
There was a problem hiding this comment.
Suggestion: Call this RunTask as you'll use it like taskResult = taskOperation.RunTask(task) #WontFix
| /// <summary> | ||
| /// The function to cancel the operation | ||
| /// </summary> | ||
| public Func<SqlTask, Task<TaskResult>> TaskToCancel |
There was a problem hiding this comment.
Suggestion: Call this CancelTask as you'll use it like taskResult = taskOperation.CancelTask(task) #WontFix
| { | ||
| ITaskOperation taskOperation = sqlTask.TaskMetadata.TaskOperation as ITaskOperation; | ||
| TaskResult taskResult = null; | ||
|
|
There was a problem hiding this comment.
Minor: remove unnecessary whitespace #Resolved
| @@ -3,6 +3,8 @@ | |||
| // Licensed under the MIT license. See LICENSE file in the project root for full license information. | |||
| // | |||
|
|
|||
| using System; | |||
|
|
||
| // create restore task and perform | ||
| SqlTask sqlTask = SqlTaskManagerInstance.CreateAndRun(metadata, this.restoreDatabaseService.RestoreTaskAsync, restoreDatabaseService.CancelTaskAsync); | ||
| SqlTask sqlTask = SqlTaskManagerInstance.CreateAndRun<SqlTask>(metadata); |
There was a problem hiding this comment.
how are we setting restore's CancelTask in SqlTask?
Any operation that's using SMO and is managed by task manager can now implement SmoScriptableTaskOperation so that the operation either execute, generate script or both
Any scriptable request can implement IScriptableRequestParams to that it gets TaskExecutionMode from client. If using the task manager, the execution mode will be passed to the sql task to either execute or generate script
Changed restore to use the new classes so that it support scripting
-TODO: I didn't refactor backup to use the new classes, I can do that next if the new contract sign off by reviewers