Skip to content

[GOBBLIN-1463] Remove pentaho dependency#3303

Merged
autumnust merged 2 commits intoapache:masterfrom
Will-Lo:remove-pentaho-dependency
Jun 9, 2021
Merged

[GOBBLIN-1463] Remove pentaho dependency#3303
autumnust merged 2 commits intoapache:masterfrom
Will-Lo:remove-pentaho-dependency

Conversation

@Will-Lo
Copy link
Copy Markdown
Contributor

@Will-Lo Will-Lo 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):

Pentaho's library for aggdesigner is used in tests. https://mvnrepository.com/artifact/org.pentaho/pentaho-aggdesigner-algorithm/5.1.5-jhyde However, the nexus repository frequently goes down and is unreliable.

Proposal to move to a newer version of aggdesigner hosted on maven central, similar to calcite https://issues.apache.org/jira/browse/CALCITE-1474.

Tests

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

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"

@Will-Lo Will-Lo changed the title Remove pentaho dependency [GOBBLIN-1463] Remove pentaho dependency Jun 9, 2021
Copy link
Copy Markdown
Contributor

@aplex aplex left a comment

Choose a reason for hiding this comment

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

LTGM. Don't forget to add motivation for the change to commit message/PR description

@codecov-commenter
Copy link
Copy Markdown

Codecov Report

Merging #3303 (e2c8627) into master (bf37c76) will decrease coverage by 3.50%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #3303      +/-   ##
============================================
- Coverage     46.50%   42.99%   -3.51%     
+ Complexity    10041     9313     -728     
============================================
  Files          2041     2041              
  Lines         79349    79349              
  Branches       8845     8845              
============================================
- Hits          36898    34117    -2781     
- Misses        39020    41995    +2975     
+ Partials       3431     3237     -194     
Impacted Files Coverage Δ
.../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%) ⬇️
...ion/mapreduce/orc/OrcKeyCompactorOutputFormat.java 0.00% <0.00%> (-100.00%) ⬇️
... and 142 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 bf37c76...e2c8627. Read the comment docs.

@Will-Lo
Copy link
Copy Markdown
Contributor Author

Will-Lo commented Jun 9, 2021

@aplex Ahh unfortunately it looks like hive-exec version also has a dependency on pentaho...need to think of how to prevent that as well.

@Will-Lo Will-Lo force-pushed the remove-pentaho-dependency branch from 8e1fca4 to 7d8ad2b Compare June 9, 2021 22:32
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

@autumnust autumnust merged commit faa8e00 into apache:master Jun 9, 2021
@Will-Lo Will-Lo deleted the remove-pentaho-dependency branch June 18, 2021 23:47
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