Repository navigation
Pk/sf/merge2 - #210
Closed
Pradeep Kadubandi (PradeepKadubandi) wants to merge 214 commits into
Closed
Pk/sf/merge2#210Pradeep Kadubandi (PradeepKadubandi) wants to merge 214 commits into
Pradeep Kadubandi (PradeepKadubandi) wants to merge 214 commits into
Conversation
…ider (no implementation yet).
…some optimization that can still be done. No timers / sub orchestrations yet - just a simple orchestration with ability to schedule tasks (activities).
…at maynot be serializable (though there's code to fix that, it's not wired up yet).
… to be able to create and wait for an orchestration. Rest of the stuff is not implemented yet.
…ther minor changes.
no real content in the projects yet. The initial intent is to do F5 from VS from the solution and hit breakpoints in some test code that tests the orchestration. Eventually this app will also be used for unit testing (by deploying the app to local cluster and making http requests from unit tests)
…oints. Eventually I will write a controller to expose the functionality and stateless app to test it and use that for unit testing.
…a separate class, we can perhaps evolve the implementation as we see fit.
…eed for service fabric implementation.
Perhaps it could be improved and also the code could be better organized?
…ions, need to test the changes though.
(User SF remoting as communication mechanism for tests) However deploying the local cluster is still a manul pre-req step for tests to run (noted as a todo). Also, at this point, the next step would be to put all existing functional tests for service bus provider into a separate dll and reuse them to test the service fabric provider to understand the gaps.
…ime to enable that functionality. For now, keeping the tests and throwing that the functionality is not supported.
//Todo: should we do something like AppendBatch to reduce the number of calls to persistent log? Above will be useful because it will reduce the number of serializations done on the objects.
However this is still using reflection (serializer.Serialize) and the perf could be improved if we want OR we have seen that the current perf is bad.
(Which should be set to false by everyone else except the service fabric provider).
… that service fabric implementation can reference those and stress test executable can reference the service fabric.
…attempted to convert the entire object. For now fixing the issue and using data contract serialization for the events as well. Added a todo to consider json serialization.
as functional tests apply for now, the code must be built and fabric application must be packaged, and also the local cluster must be started before the test is run. Also the the task output is not really reliable.
…stration with same instanceid could be created while another is running (if there is high enough load and there are concurrent requests to CreateOrchestration with the same instance id). This fix simplifies existing orchestration check and uses TryAdd in the transaction itself instead of ContainsKey check as part of a previous transaction. Also harden the existing test to catch the issue (the test breaks without the code fix and runs fine after the code fix!!!)
…ementation class.
…ot work. It looks like the framework processing of orchestration cannot handle this scenario. (Need to enable the test once the issue is figured out and fixed)
…ten stopping of program when locking the machine or machine going to sleep)
…pdate' from ConcurrentDictionary. from concurrent dictionary. (AddOrUpdate is not atomic because the update value factory is executed outside the lock in concurrent dictionary, I have seen with a minimized repro that this is somehow causing a session to be skipped from getting enqueued in the fetch queue - though I know that it's becase of my use of a local variable that gets set inside the update value factory and used to decide whether to add to fetch queue or not, I am still not 100% sure how might that happen). But with my minimized repro program, I have seen that this fix makes it not repro. (Ran it a bunch of times to make sure). I am expecting this is the same bug that is causing a few stuck orchestrations in messaging stress.
… fabric app. I am adding any traces above Warning be defult ... on demand the switch can be changed if we need vesrbose logging.
…ent before the orchestration is picked up by dispatcher leads to a blocked orchestration (the processing keeps abandoning the orchestration). (#113) Only the first terminate request is honored and subsequent terminates are ignored. (That makes the fix simple and hopefully not a problematic behavior from user's perspective)
…aster and cherry-picked here, I can enable the test.
…lue. (Fabric will throw an error anyway and upper layer can read the correct error and respond instead of reading OperationCancelledError)
… pk/sf/merge2) # Conflicts: # Test/DurableTask.Test.Orchestrations/DurableTask.Test.Orchestrations.csproj # samples/DurableTask.Samples/DurableTask.Samples.csproj # samples/DurableTask.Samples/packages.config # src/DurableTask.Core/Settings/TaskActivityDispatcherSettings.cs # src/DurableTask.Core/Settings/TaskOrchestrationDispatcherSettings.cs # src/DurableTask.Framework/DurableTask.Framework.csproj # src/DurableTask.Framework/Properties/AssemblyInfo.cs # src/DurableTask.Framework/packages.config # test/DurableTask.Framework.Tests/DurableTask.Framework.Tests.csproj # test/DurableTask.Framework.Tests/app.config # test/DurableTask.Framework.Tests/packages.config # test/DurableTask.ServiceBus.Tests/DurableTask.ServiceBus.Tests.csproj # test/DurableTask.ServiceBus.Tests/packages.config # test/DurableTask.Stress.Tests/DurableTask.Stress.Tests.csproj # test/DurableTask.Stress.Tests/packages.config
Pradeep Kadubandi (PradeepKadubandi)
requested a review
from Simon Porter (simonporter)
August 23, 2018 21:08
Contributor
Author
|
I am attempting the conversion to .Net Standard (because I realized I need that to build my project anyways) - hold on reviewing this till I update the PR with more commits. |
| @@ -0,0 +1,118 @@ | |||
| <?xml version="1.0" encoding="utf-8"?> | |||
| <Project ToolsVersion="14.0" DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003"> | |||
Contributor
There was a problem hiding this comment.
We should at least switch to the new csproj format, would be really weird bring in the old format to master at this point.
| <Private>True</Private> | ||
| </Reference> | ||
| <Reference Include="Microsoft.ServiceFabric.Internal.Strings"> | ||
| <HintPath>..\..\packages\Microsoft.ServiceFabric.5.5.216\lib\net45\Microsoft.ServiceFabric.Internal.Strings.dll</HintPath> |
Contributor
There was a problem hiding this comment.
This version is quite old, can this be udaed to newer?
| // You can specify all the values or you can default the Build and Revision Numbers | ||
| // by using the '*' as shown below: | ||
| // [assembly: AssemblyVersion("1.0.*")] | ||
| [assembly: AssemblyVersion("1.0.0.0")] |
Contributor
There was a problem hiding this comment.
You will need to confirm it can build signed, similar to this: https://github.com/Azure/durabletask/blob/master/src/DurableTask.Core/AssemblyInfo.cs
Contributor
Author
There was a problem hiding this comment.
Not sure if you meant that this file should include DurableTask.props MSBuild definition file OR I should wrap any InternalsVisibleTo attrributes inside the SIGN_ASSEMBLY build flag:
- There's only one place where I used InternalsVisibleTo flag in my code and I now wrapped this within SIGN_ASSEMBLY flag so that it will be skipped when building for signing.
- If you meant that my binaries need to include thee msbuild file, I am curious on why? These are test binaries that are only meant for development and testing and not for release. (The file that you commented on)
| using Owin; | ||
| using TestApplication.Common; | ||
|
|
||
| namespace TestStatefulService |
Contributor
There was a problem hiding this comment.
can we use a better namespace?
…mblies for build.
… across executions is still not supported though). Detailed commit notes/history below: Squashed commit of the following: commit 8d31731 Author: Pradeep Kadubandi <pradeep.v.kadubandi@gmail.com> Date: Thu Jun 20 01:24:02 2019 +0000 Fixing a minor overlook. commit 26362eb Author: Pradeep Kadubandi <pradeep.v.kadubandi@gmail.com> Date: Wed Jun 19 23:35:18 2019 +0000 After reviewing the service bus client functionality for WaitForOrchestation client API which ignores the input executionId and instead wait for latest execution id of given orchestration, I changed the fabric provider client's implementation to match that. The fabric provider still does not support querying for orchestration state across all executions. To implement that, we perhaps needs to introduce one more dictionary to track all execution ids for a given orchestration instance. There are some open questions about this like should we differentiate the executions from ContinueAsNew to that of different executions caused by user creating the orchestration with same instanceId after the current one completes. Also it's not clear if this functionality is desired at all. So until I hear otherwise, I choose to postpone it. commit 5a79296 Author: Pradeep Kadubandi <pradeep.v.kadubandi@gmail.com> Date: Tue Jun 18 01:25:03 2019 +0000 More changes after testing the Contnued Orchestration Functionality. These changes work on the TaskHub side (i.e., Continued orchestration runs as expected) as seen in debugging. However, the instance store still requires some changes - for ContinueAsNew support to make sense, we probably should support querying for state across all executions? commit 6f0fa96 Author: Pradeep Kadubandi <pradeep.v.kadubandi@gmail.com> Date: Mon Jun 17 22:53:52 2019 +0000 Initial change to enable ContinueAsNew. Also enabling the existing basic functional test for ContinusAsNew. There are still at least 2 issues to follow through: - It seems to me that the old execution should be fine. The status of Orchestration when queried for that specific execution comes out as 'ContinuedAsNew' which is probably correct. However, I still need to confirm that the new execution works as expected (with the current test, as soon as old execution is completed, the test exits killing the fabric application making it harder to understand). - The instance store does not support traking the state over multiple executions. Without this, there is no way to get the latest history for a given orchstration with one instance id and latest execution.
Member
|
Closing stale PRs. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is just a merge and resolve conflicts (most conflict resolution was accepting master changes).
There are a few follow ups needed (I am thinking we can do that after the merge):