This repository has been archived by the owner on Dec 13, 2023. It is now read-only.
Workflow/Task Summary Input/Output Json Serialization #2128
Conversation
This file contains 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
…ed Workflow/TaskSummary classes and the server's ModulesProvider to leverage the configuration, which allows the summary input/output strings to be serialized as JSON strings as opposed to the default configuration of using Java Map toString()
…management and added in tests for the additional features as well as a regression test for expected default behavior
apanicker-nflx
approved these changes
Mar 16, 2021
Codecov Report
@@ Coverage Diff @@
## dev #2128 +/- ##
============================================
- Coverage 65.78% 65.76% -0.02%
- Complexity 4087 4091 +4
============================================
Files 309 310 +1
Lines 19252 19270 +18
Branches 1758 1758
============================================
+ Hits 12665 12673 +8
- Misses 5720 5730 +10
Partials 867 867
Continue to review full report at Codecov.
|
Pull Request Test Coverage Report for Build 5173
💛 - Coveralls |
dsmith-globys
deleted the
feature/summary_input_output_json_serialization
branch
TwoUnderscorez
pushed a commit
to TwoUnderscorez/conductor
that referenced
this pull request
Jul 23, 2021
* Added configuration for SummaryInputOutputJSONSerialization and updated Workflow/TaskSummary classes and the server's ModulesProvider to leverage the configuration, which allows the summary input/output strings to be serialized as JSON strings as opposed to the default configuration of using Java Map toString() * Updated Task/Workflow Summary Input/Output Json String configuration management and added in tests for the additional features as well as a regression test for expected default behavior * Minor whitespace fix in WorkflowSummary * Another tiny whitespace tweak for WorkflowSummary * Minor whitespace changes to SummaryUtil
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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 feature allows for users to configure the string serialization method used for WorkflowSummary and TaskSummary Input/Output objects when *Summary classes are created either for storing in ElasticSearch or when being pulled back from ElasticSearch.
For my company's current use-case, all of our microservices which interact with the conductor server API we have running are written in JavaScript, so having the *Summary class Input/Output strings be JSON-serialized means we can trivially interact with the *Summary types in JS instead of trying to hand-parse Java's toString() serialization. Searching for Workflows and Tasks by fields of the input still works as expected regardless of serialization mechanism.
The default Java toString() methodology is kept in place, so the change will not affect any users who do not intentionally enable the summaryInputOutputJsonSerializationEnabled configuration. The configurations were added into the standard server.properties and config.properties (commented out with a note on what they do), and a Unit Test was written to flex the new logic and ensure everything is copacetic.
*NOTE: My original implementation wanted to try and use the Configuration class directly in SummaryUtil, but given the /common/ code is a leaf in the dependency tree and /core/, which has Configuration, is higher in the dependency tree, I had to avoid the dependency circle. To do so, I inserted the BootstrapUtil.configureSummaryUtil call into the Server's main startup function. Please let me know if there would be a better location/manner to handle this, but after trying a few different things, what I currently have seemed the cleanest. I'm more than happy to make adjustments if this doesn't quite meet the standards.