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

Refactor: Split UdfLoader into multiple files #3689

Merged
merged 2 commits into from
Oct 30, 2019

Conversation

purplefox
Copy link
Contributor

@purplefox purplefox commented Oct 28, 2019

Description

This is the 4th PR in a stack. The previous PR is #3687. please don't review the commits from the previous PR you will see in here.

This PR refactors UdfLoader to be more manageable - it was becoming a sprawling mega-class that did too much and suffered from poor cyclomatic complexity.

There is more or less zero new code in this PR - it pretty much extracts existing code to new classes and does some renaming.

Now:

  • UdfLoader loads udfs
  • UdafLoader loads udafs
  • UdtfLoader loads udtfs
  • UserFunctionLoader is the corordinator for the overall loading process
  • FunctionLoaderUtils contains static util functions used by the different loaders
  • UdfLoader Passes checkstyle cyclomatic complexity check (previously it didn't)

Testing done

Non functional change.

Reviewer checklist

  • Ensure docs are updated if necessary. (eg. if a user visible feature is being added or changed).
  • Ensure relevant issues are linked (description should include text like "Fixes #")

@purplefox purplefox requested a review from a team as a code owner October 28, 2019 14:44
@purplefox purplefox requested a review from agavra October 28, 2019 17:13
@purplefox purplefox force-pushed the split_udf_loader_4 branch 3 times, most recently from 5234f22 to 35824c4 Compare October 29, 2019 16:25
Copy link
Contributor

@agavra agavra left a comment

Choose a reason for hiding this comment

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

I skimmed this PR to make sure the code structure made sense to me, I didn't look at the guts of the classes sine I trust that it was all moved code (as you mentioned). Thanks @purplefox!

@agavra agavra requested a review from a team October 30, 2019 18:07
@purplefox purplefox merged commit 63d2be6 into confluentinc:master Oct 30, 2019
purplefox added a commit that referenced this pull request Oct 30, 2019
This commit refactors UdfLoader to be more manageable - it was becoming a sprawling mega-class that did too much and suffered from poor cyclomatic complexity.

There is more or less zero new code in this PR - it pretty much extracts existing code to new classes and does some renaming.

Now:

UdfLoader loads udfs
UdafLoader loads udafs
UdtfLoader loads udtfs
UserFunctionLoader is the corordinator for the overall loading process
FunctionLoaderUtils contains static util functions used by the different loaders
UdfLoader Passes checkstyle cyclomatic complexity check (previously it didn't)
@purplefox purplefox deleted the split_udf_loader_4 branch January 26, 2020 22:12
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.

2 participants