[GOBBLIN-1497] Reduce the number of calls on FlowSpec initialization where possible,… #3340
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
Codecov Report
@@ Coverage Diff @@
## master #3340 +/- ##
============================================
- Coverage 46.54% 46.54% -0.01%
- Complexity 10136 10137 +1
============================================
Files 2051 2052 +1
Lines 79547 79563 +16
Branches 8880 8882 +2
============================================
+ Hits 37027 37030 +3
- Misses 39091 39102 +11
- Partials 3429 3431 +2
Continue to review full report at Codecov.
|
… and configToProperties/vice versa
Will-Lo
force-pushed
the
optimize-config-properties
branch
from
7181b29
to
4e38fdc
Compare
|
@arjun4084346 @jack-moseley @umustafi could you take a look? |
gobblin-service/src/main/java/org/apache/gobblin/service/modules/restli/FlowConfigUtils.java
Outdated
Show resolved
Hide resolved
gobblin-utility/src/main/java/org/apache/gobblin/util/PropertiesUtils.java
Outdated
Show resolved
Hide resolved
|
minor comments to simply things. LGTM otherwise. good changes. maybe we should get rid of configAsProperties entirely. but that may need more changes. |
|
@arjun4084346 yea we should look into configAsProperties especially at the rate that GaaS is growing at. Would require some refactoring around some legacy classes and probably more conversions from propertiesToConfig. However since those would be short-lived compared to FlowSpecs it would still be an overall memory improvement |
umustafi
approved these changes
Jul 27, 2021
sv2000
reviewed
Jul 28, 2021
...e/src/main/java/org/apache/gobblin/service/modules/scheduler/GobblinServiceJobScheduler.java
Outdated
Show resolved
Hide resolved
phet
pushed a commit
to phet/gobblin
that referenced
this pull request
Aug 9, 2021
…where possible,… (apache#3340) * Reduce the number of calls on FlowSpec initialization where possible, and configToProperties/vice versa * address review * Address comments
jack-moseley
pushed a commit
to jack-moseley/gobblin
that referenced
this pull request
Aug 24, 2022
…where possible,… (apache#3340) * Reduce the number of calls on FlowSpec initialization where possible, and configToProperties/vice versa * address review * Address comments
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.
… and configToProperties/vice versa
Dear Gobblin maintainers,
Please accept this PR. I understand that it will not be reviewed until I have checked off all the steps below!
JIRA
Description
FlowSpecs contain both configs and property fields that duplicate the same key/value information, however there are slight differences with properties being mutable, configs being type-safe, and legacy code being compatible with properties but not configs.
As such, FlowSpec creation and configToProperties are memory expensive operations at scale, and we should aim to minimize them as much as possible.
Tests
Commits