Skip to content

GOBBLIN-1462: Ensure FsSpecConsumer handles config keys which are prefixes of other keys#3302

Closed
sv2000 wants to merge 2 commits intoapache:masterfrom
sv2000:typesafeConfigPrefix
Closed

GOBBLIN-1462: Ensure FsSpecConsumer handles config keys which are prefixes of other keys#3302
sv2000 wants to merge 2 commits intoapache:masterfrom
sv2000:typesafeConfigPrefix

Conversation

@sv2000
Copy link
Copy Markdown
Contributor

@sv2000 sv2000 commented Jun 9, 2021

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

  • Here are some details about my PR, including screenshots (if applicable):
    Currently, the FsSpecConsumer fails when building a JobSpec if the configuration contains keys which are prefixes of other keys, which is not allowed in typesafe config. In practice, this constraint may be violated particularly when loading system properties which are outside application's control. To ensure these properties are safely handled, the FsSpecConsumer should use the proertiesToConfig() method in PropertiesUtils which adds a suffix to the root properties.

Tests

  • My PR adds the following unit tests OR does not need testing for this extremely good reason:
    Modified unit tests

Commits

  • My commits all reference JIRA issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "How to write a good git commit message":
    1. Subject is separated from body by a blank line
    2. Subject is limited to 50 characters
    3. Subject does not end with a period
    4. Subject uses the imperative mood ("add", not "adding")
    5. Body wraps at 72 characters
    6. Body explains "what" and "why", not "how"

jobSpecBuilder.withJobCatalogURI(avroJobSpec.getUri())
.withVersion(avroJobSpec.getVersion())
.withDescription(avroJobSpec.getDescription())
.withConfigAsProperties(props)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we still need this line?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we will still need this line. The only change is we are now explicit about how the config member variable of JobSpec is set. Previously, it defaults to using ConfigUtils#propertiesToTypedConfig() which throws an exception when it encounters prefixed keys.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This API is quite confusing ... But I get the point why both will be needed.

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Jun 9, 2021

Codecov Report

Merging #3302 (78cb1f6) into master (5199078) will decrease coverage by 3.46%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #3302      +/-   ##
============================================
- Coverage     46.46%   43.00%   -3.47%     
+ Complexity    10023     9316     -707     
============================================
  Files          2037     2041       +4     
  Lines         79291    79352      +61     
  Branches       8840     8845       +5     
============================================
- Hits          36845    34124    -2721     
- Misses        39031    41990    +2959     
+ Partials       3415     3238     -177     
Impacted Files Coverage Δ
...org/apache/gobblin/runtime/api/FsSpecConsumer.java 62.06% <100.00%> (+2.06%) ⬆️
.../org/apache/gobblin/util/filters/HiddenFilter.java 0.00% <0.00%> (-100.00%) ⬇️
...g/apache/gobblin/cluster/HelixMessageSubTypes.java 0.00% <0.00%> (-100.00%) ⬇️
...gobblin/runtime/mapreduce/GobblinOutputFormat.java 0.00% <0.00%> (-100.00%) ⬇️
...obblin/compaction/source/CompactionFailedTask.java 0.00% <0.00%> (-100.00%) ⬇️
...n/cluster/event/ClusterManagerShutdownRequest.java 0.00% <0.00%> (-100.00%) ⬇️
...in/compaction/action/CompactionCompleteAction.java 0.00% <0.00%> (-100.00%) ⬇️
...n/compaction/mapreduce/orc/OrcKeyDedupReducer.java 0.00% <0.00%> (-100.00%) ⬇️
...action/audit/KafkaAuditCountHttpClientFactory.java 0.00% <0.00%> (-100.00%) ⬇️
...askStateCollectorServiceHiveRegHandlerFactory.java 0.00% <0.00%> (-100.00%) ⬇️
... and 172 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5199078...78cb1f6. Read the comment docs.

Copy link
Copy Markdown
Contributor

@ZihanLi58 ZihanLi58 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, LGTM

Copy link
Copy Markdown
Contributor

@autumnust autumnust left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 LGTM

jobSpecBuilder.withJobCatalogURI(avroJobSpec.getUri())
.withVersion(avroJobSpec.getVersion())
.withDescription(avroJobSpec.getDescription())
.withConfigAsProperties(props)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This API is quite confusing ... But I get the point why both will be needed.

@sv2000 sv2000 closed this Jun 9, 2021
@sv2000 sv2000 reopened this Jun 9, 2021
@sv2000 sv2000 closed this Jun 10, 2021
@sv2000 sv2000 reopened this Jun 10, 2021
@sv2000 sv2000 force-pushed the typesafeConfigPrefix branch from ca2940b to 2e77de0 Compare June 10, 2021 18:43
@asfgit asfgit closed this in 1bc7e82 Jun 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants