Skip to content

Comments

SAMZA-2210: Initial majority migration for injecting classloader when doing reflection #1047

Merged
cameronlee314 merged 27 commits intoapache:masterfrom
cameronlee314:initial_majority_cl
Jun 5, 2019
Merged

SAMZA-2210: Initial majority migration for injecting classloader when doing reflection #1047
cameronlee314 merged 27 commits intoapache:masterfrom
cameronlee314:initial_majority_cl

Conversation

@cameronlee314
Copy link
Contributor

@cameronlee314 cameronlee314 commented May 22, 2019

When doing reflection, we will now explicitly specify the classloader to use. This will allow us to inject a custom classloader in the future, in order to do things like isolate Samza infrastructure from application code.
From a functionality perspective, this should not change anything. Ultimately, for this PR, the classloader that is passed through is the same classloader that is already being used. The purpose of this PR is to do the wiring to allow easier evolution in a follow-up PR.
This does not cover all reflection cases. There are several other follow-up PRs which will handle other reflection cases. This PR is already very significantly large, so trying to keep it from being even larger.

This PR includes #1044.

… more consistent with other ReflectionUtil methods
…er to be more consistent with other ReflectionUtil methods"

This reverts commit ad96515.

The commit being reverted should have been made on the move_reflection_utils
branch, so will re-apply through that branch.
…ther PR comments"

This reverts commit bb99c36.

The commit being reverted should have been made on the move_reflection_utils
branch, so will re-apply through that branch.
… more consistent with other ReflectionUtil methods
… more consistent with other ReflectionUtil methods
… migrate unnecessary usages of getObjWithArgs
Copy link
Contributor

@mynameborat mynameborat left a comment

Choose a reason for hiding this comment

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

Looks good.
One general question around code style, I noticed some of the methods/callsite have one argument per line while others have them crunched together to fit the width limit. This is prevalent across the code base but I noticed you changed some of them too. So wondering what the guideline is?

@cameronlee314
Copy link
Contributor Author

Looks good.
One general question around code style, I noticed some of the methods/callsite have one argument per line while others have them crunched together to fit the width limit. This is prevalent across the code base but I noticed you changed some of them too. So wondering what the guideline is?

My personal preference is that "long" argument lists have the format of one argument per line. "Long" can be pretty subjective, but I usually consider argument lists that take up 3 or more lines as "long". The reason why I prefer this style is because future changes to the argument lists are easier to diff (e.g. diff has a single-line addition instead of possibly having multiple lines updated due to overflow of line length). I will sometimes make those style changes when I need to change an argument list.
All that said, I'm not sure what the general Samza style guideline is around this.

@prateekm
Copy link
Contributor

prateekm commented Jun 3, 2019

+1 for Cameron's suggestion on long param list formatting. My "long" threshold is higher (6-7 params / 1-2 lines), but I also prefer to keep/make longer lists multi-line.

@vjagadish1989
Copy link
Contributor

+1;

  1. In general, function signatures can have the same line-width as other lines in the source-file.
  2. When introducing line breaks, don't break a param definition half-way

@mynameborat
Copy link
Contributor

Looks good.
One general question around code style, I noticed some of the methods/callsite have one argument per line while others have them crunched together to fit the width limit. This is prevalent across the code base but I noticed you changed some of them too. So wondering what the guideline is?

My personal preference is that "long" argument lists have the format of one argument per line. "Long" can be pretty subjective, but I usually consider argument lists that take up 3 or more lines as "long". The reason why I prefer this style is because future changes to the argument lists are easier to diff (e.g. diff has a single-line addition instead of possibly having multiple lines updated due to overflow of line length). I will sometimes make those style changes when I need to change an argument list.
All that said, I'm not sure what the general Samza style guideline is around this.

Sounds good! We have a coding guideline that touches some areas but not comprehensive. At least that is the my motivation behind these guidelines/style questions. I'd be happy to consolidate our guidelines into a comprehensive wiki and share it.
Let me know what do you think?

Copy link
Contributor

@prateekm prateekm left a comment

Choose a reason for hiding this comment

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

General questions about whether naming the fields / params / variables more explicitly (fwkClassloader vs pluginClassloader) help clarify the expected usage.

Some other general questions about whether a fwk or plugin classloader should be used.

Otherwise LGTM, thanks for the cleanup as well.

Copy link
Contributor

@prateekm prateekm left a comment

Choose a reason for hiding this comment

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

LGTM, thanks. Feel free to merge after updating with the 'extract the side inputs processor' comment.

Copy link
Contributor

@prateekm prateekm left a comment

Choose a reason for hiding this comment

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

Thanks!

@cameronlee314 cameronlee314 merged commit 3b87fbd into apache:master Jun 5, 2019
@cameronlee314 cameronlee314 deleted the initial_majority_cl branch October 4, 2019 21:42
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