Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[HUDI-1211] clean up spark session for each test of FunctionalTestHar… #2697

Closed
wants to merge 1 commit into from

Conversation

kwondw
Copy link
Contributor

@kwondw kwondw commented Mar 19, 2021

…ness

Tips

What is the purpose of the pull request

To fix the issue of HUDI-1211,

Brief change log

  • close spark session after each test in FunctionalTestHarness.java

Verify this pull request

  • This change is only for tests

Committer checklist

  • Has a corresponding JIRA in PR title & commit

  • Commit message is descriptive of the change

  • CI is green

  • Necessary doc changes done or have another open PR

  • For large changes, please consider breaking it into sub-tasks under an umbrella JIRA.

@codecov-io
Copy link

codecov-io commented Mar 19, 2021

Codecov Report

Merging #2697 (3c88186) into master (161d530) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##             master   #2697   +/-   ##
========================================
  Coverage      9.55%   9.55%           
  Complexity       48      48           
========================================
  Files            53      53           
  Lines          1958    1958           
  Branches        233     233           
========================================
  Hits            187     187           
  Misses         1758    1758           
  Partials         13      13           
Flag Coverage Δ Complexity Δ
hudiutilities 9.55% <ø> (ø) 0.00 <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

@vinothchandar
Copy link
Member

@kwondw this is done intentionally to speed up tests.

cc @xushiyan can you please comment on this ?

@xushiyan xushiyan self-assigned this Mar 23, 2021
@xushiyan xushiyan self-requested a review March 23, 2021 18:50
Copy link
Member

@xushiyan xushiyan left a comment

Choose a reason for hiding this comment

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

I would like to understand if the linked issue was reproducible and solved by this change.

The original intention was to share spark session for test cases in each module that has functional test suite configured. And all test cases tied to the functional test suite are ran sequentially so the shared spark session should be okay. In the maven profile functional-tests, we configured these to ensure one new JVM process to execute all tests in one Maven module.

<forkCount>1</forkCount>
<reuseForks>true</reuseForks>

I'm not against this change, though it looks like will be doing many more spark init, just want to understand what exactly the issue was and how this fixed it. Thanks.

@nsivabalan
Copy link
Contributor

fyi, we did merge a hot fix couple of days back on similar lines. locally, both funcational and Hudi-utilities tests ran parallel and we ran into some issues.

@nsivabalan nsivabalan added code-clean priority:major degraded perf; unable to move forward; potential bugs labels Mar 30, 2021
@vinothchandar vinothchandar added this to Ready For Review in PR Tracker Board Apr 15, 2021
@vinothchandar
Copy link
Member

@nsivabalan are you saying this pr is already subsumed by other fixes on master? Can you please clarify what your suggestion for this pr is

Copy link
Contributor

@nsivabalan nsivabalan left a comment

Choose a reason for hiding this comment

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

@kwondw : am in line with @xushiyan. Don't think we need to close and init resources for each test. If you see any issues by one time instantiation and tear down per class, let us know. we can consider the fix. If not, would prefer to leave it that way as this might increase our (test execution/)running time as well.

@vinothchandar
Copy link
Member

Closing due to inactivity

PR Tracker Board automation moved this from Under Discussion PRs to Done Sep 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority:major degraded perf; unable to move forward; potential bugs
Projects
Development

Successfully merging this pull request may close these issues.

None yet

6 participants