Refactor code base to remove dependency on 'Microsoft.Extensions.Logging' and merge 'Function' with 'FunctionInfo' #22

daxian-dbw · 2018-08-31T07:01:42Z

Refactor the code base to remove the dependency on Microsoft.Extensions.Logging and merge Function with FunctionInfo.

Since we don't pass the logger to the user, it doesn't need to implement ILogger. Removing this dependency reduce 8 assemblies from the publish folder.
The type Function is not really needed, so merge it to FunctionInfo.
Make Params, Headers and Query in HttpRequestContext case insensitive. So $req.Query.Name works for ?name=Joe.

TylerLeonhardt

Looks good. Just one small suggestion.

TylerLeonhardt · 2018-08-31T15:32:11Z

src/RequestProcessor.cs

+            }
+
+            var scriptPath = functionInfo.ScriptPath;
+            var entryPoint = functionInfo.EntryPoint;


nit: scriptPath and entryPoint are only used once. Maybe just change the InvokeFunction call to:

result = _powerShellManager.InvokeFunction( functionInfo.ScriptPath, functionInfo.EntryPoint, triggerMetadata, invocationRequest.InputData);

TylerLeonhardt · 2018-08-31T15:34:45Z

src/RequestProcessor.cs

@@ -150,12 +136,38 @@ internal StreamingMessage ProcessInvocationRequest(StreamingMessage request)
                }
            };

+            // Load information about the function
+            var functionInfo = _functionLoader.GetFunctionInfo(invocationRequest.FunctionId);
+            if (functionInfo == null)


I think it would make more sense if this would throw an exception rather than returning null if it couldn't find the function.

Then we can include the exception in the status that is sent back.

TylerLeonhardt · 2018-08-31T15:37:02Z

src/RequestProcessor.cs

@@ -165,21 +177,22 @@ internal StreamingMessage ProcessInvocationRequest(StreamingMessage request)
            }

            // Set out binding data and return response to be sent back to host
-            foreach (KeyValuePair<string, BindingInfo> binding in functionInfo.OutputBindings)
+            foreach (KeyValuePair<string, BindingInfo> binding in functionInfo.OutBindings)


this change reminds me that we should be consistent in our wording. I've seen the use of out bindings and output bindings. Our code should be consistent.

Anyway, you don't need to address this in your PR.

I searched the code base, the generated protocol code has mixed uses of out binding and output binding. In our code, we uses outputbinding in the helper module, while outbinding in worker. I guess outbinding is the offcial AzF term. Maybe we change the function name to push-outbinding?

@asavaritayal can you comment on this?

Would it make more sense in the AzF world to call everything:

outputbinding or outbinding?

AzF documents use input and output bindings.

I see. We should change funnctionInfo.OutBinding tofunctionInfo.OutputBinding then, I think.

~~Will make the change.~~ Done.

daxian-dbw · 2018-08-31T20:23:43Z

I have tested the worker locally. All xunit tests passed.

TylerLeonhardt

LGTM just one small nit

TylerLeonhardt · 2018-08-31T20:46:12Z

src/Http/HttpRequestContext.cs

@@ -4,15 +4,26 @@
 //

 using System;
+using System.Collections.Generic;
 using Google.Protobuf.Collections;


nit: we shouldn't need this using anymore

We need it for the use of Dictionary<string, string> in HttpRequestContext. MapField<string, string> is case sensitive, we need case insensitive for powershell, so $req.Query.Name works for ?name=Joe.

Yes I know. I meant remove Google.Protobuf.Collections since you removed the references to MapField

Got it. Will submit another PR to remove it.

# This is the 1st commit message: separate DF SDK classes from DF worker classes # This is the commit message #2: fix typo # This is the commit message #3: DurableSDK now compiles by itself # This is the commit message #4: Allow ExternalSDK to handle orchestration # This is the commit message #5: document next steps # This is the commit message #6: allow external SDK to set the user-code's input. Still need to refactor this logic for the worker to continue working with old SDK # This is the commit message #7: add import module # This is the commit message #8: supress traces # This is the commit message #9: avoid nullptr # This is the commit message #10: pass tests # This is the commit message #11: fix E2E tests # This is the commit message #12: develop E2E tests # This is the commit message #13: Enabled external durable client (#765) Co-authored-by: Michael Peng <[email protected]> # This is the commit message #14: bindings work # This is the commit message #15: conditional binding intialization # This is the commit message #16: conditional import # This is the commit message #17: Added exception handling logic # This is the commit message #18: Revert durableController name to durableFunctionsUtils # This is the commit message #19: Ensure unit tests are functioning properly # This is the commit message #20: Corrected unit test names # This is the commit message #21: Turned repeated variables in unit tests into static members # This is the commit message #22: Fixed issue with building the worker # This is the commit message #23: Fix E2E test # This is the commit message #24: Fixed unit test setup # This is the commit message #25: Fixed another unit test setup # This is the commit message #26: Remove string representation of booleans # This is the commit message #27: patch e2e test # This is the commit message #28: remove typo in toString # This is the commit message #29: Update PowerShell language worker pipelines (#750) * Install .Net to a global location * Remove .Net installation tasks * Update install .Net 6 task * Update Windows image to use windows-latest # This is the commit message #30: Make throughput warning message visible for tooling diagnosis (#757) # This is the commit message #31: Update grpc.tools to version 2.43.0 # This is the commit message #32: Update Google.Protobuf.Tools to version 3.19.4 # This is the commit message #33: Revert "Update Google.Protobuf.Tools to version 3.19.4" This reverts commit bcbd022. # This is the commit message #34: Revert "Update grpc.tools to version 2.43.0" This reverts commit ccb323a. # This is the commit message #35: Update Google.Protobuf to 3.19.4 and grpc.tools to 2.43.0 (#762) * Update grpc.tools to version 2.43.0 * Update Google.Protobuf.Tools to version 3.19.4 # This is the commit message #36: Switch from Grpc.Core to Grpc.Net.Client (#758) * Upgraded protobuf versions and removed Grpc.Core dependency * Updated channel and option types used * Change channel credentials * Added http prefix to url * Add valid URL check and explicitly include credentials # This is the commit message #37: Update pipeline logic to generate the SBOM for release builds (#767)

Refactor code base

fdd3202

daxian-dbw requested a review from TylerLeonhardt August 31, 2018 07:01

TylerLeonhardt reviewed Aug 31, 2018

View reviewed changes

Make params, headers and queries case-insensitive

c0ca5a5

TylerLeonhardt approved these changes Aug 31, 2018

View reviewed changes

Rename 'OutBinding' to 'OutputBinding'

463a06c

daxian-dbw merged commit 4af878f into Azure:dev Aug 31, 2018

daxian-dbw deleted the mywork branch August 31, 2018 21:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor code base to remove dependency on 'Microsoft.Extensions.Logging' and merge 'Function' with 'FunctionInfo' #22

Refactor code base to remove dependency on 'Microsoft.Extensions.Logging' and merge 'Function' with 'FunctionInfo' #22

daxian-dbw commented Aug 31, 2018 •

edited

Loading

TylerLeonhardt left a comment

TylerLeonhardt Aug 31, 2018

TylerLeonhardt Aug 31, 2018

TylerLeonhardt Aug 31, 2018

daxian-dbw Aug 31, 2018

TylerLeonhardt Aug 31, 2018

daxian-dbw Aug 31, 2018

TylerLeonhardt Aug 31, 2018

daxian-dbw Aug 31, 2018 •

edited

Loading

daxian-dbw commented Aug 31, 2018

TylerLeonhardt left a comment

TylerLeonhardt Aug 31, 2018

daxian-dbw Aug 31, 2018

TylerLeonhardt Aug 31, 2018

daxian-dbw Aug 31, 2018

Refactor code base to remove dependency on 'Microsoft.Extensions.Logging' and merge 'Function' with 'FunctionInfo' #22

Refactor code base to remove dependency on 'Microsoft.Extensions.Logging' and merge 'Function' with 'FunctionInfo' #22

Conversation

daxian-dbw commented Aug 31, 2018 • edited Loading

TylerLeonhardt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daxian-dbw Aug 31, 2018 • edited Loading

Choose a reason for hiding this comment

daxian-dbw commented Aug 31, 2018

TylerLeonhardt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daxian-dbw commented Aug 31, 2018 •

edited

Loading

daxian-dbw Aug 31, 2018 •

edited

Loading