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.

Sign up for GitHub

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

Jump to bottom

Refactor BasicAuthUtils from pinot-core to pinot-common #11620

Merged
merged 1 commit into from Sep 19, 2023
Merged

Refactor BasicAuthUtils from pinot-core to pinot-common #11620

merged 1 commit into from Sep 19, 2023

Conversation

xiangfu0
Copy link
Contributor

@xiangfu0 xiangfu0 commented Sep 19, 2023 •
edited

  • Refactor BasicAuthUtils from pinot-core to pinot-common
  • Remove pinot-core dependency from pinot-jdbc-client
  • Format a few Auth files

@xiangfu0 xiangfu0 mentioned this pull request Sep 19, 2023
Open
@xiangfu0 xiangfu0 added dependencies Pull requests that update a dependency file pinot-client refactor labels Sep 19, 2023
@codecov-commenter
Copy link

codecov-commenter commented Sep 19, 2023 •
edited

Codecov Report

Merging #11620 (9f40d58) into master (e123642) will decrease coverage by 0.11%.
The diff coverage is 2.01%.

@@             Coverage Diff              @@
##             master   #11620      +/-   ##
============================================
- Coverage     63.05%   62.94%   -0.11%     
  Complexity     1105     1105              
============================================
  Files          2325     2326       +1     
  Lines        124913   124916       +3     
  Branches      19146    19146              
============================================
- Hits          78758    78623     -135     
- Misses        40533    40656     +123     
- Partials       5622     5637      +15
Flag Coverage Δ
integration <0.01% <0.00%> (ø)
integration1 <0.01% <0.00%> (ø)
integration2 0.00% <0.00%> (ø)
java-11 14.48% <2.01%> (-48.53%) ⬇️
java-17 62.90% <2.01%> (+<0.01%) ⬆️
java-20 14.52% <2.01%> (-48.41%) ⬇️
temurin 62.94% <2.01%> (-0.11%) ⬇️
unittests 62.93% <2.01%> (-0.11%) ⬇️
unittests1 67.08% <0.00%> (-0.19%) ⬇️
unittests2 14.52% <2.01%> (+0.02%) ⬆️

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

Files Changed Coverage Δ
...broker/broker/ZkBasicAuthAccessControlFactory.java 0.00% <0.00%> (ø)
...a/org/apache/pinot/common/auth/BasicAuthUtils.java 0.00% <0.00%> (ø)
...ller/api/access/BasicAuthAccessControlFactory.java 0.00% <0.00%> (ø)
...er/api/access/ZkBasicAuthAccessControlFactory.java 0.00% <0.00%> (ø)
...ava/org/apache/pinot/core/auth/BasicAuthUtils.java 0.00% <0.00%> (ø)
...he/pinot/server/access/BasicAuthAccessFactory.java 0.00% <0.00%> (ø)
.../pinot/server/access/ZkBasicAuthAccessFactory.java 0.00% <0.00%> (ø)
...ava/org/apache/pinot/client/utils/DriverUtils.java 19.09% <33.33%> (ø)
...t/broker/broker/BasicAuthAccessControlFactory.java 83.33% <100.00%> (+0.47%) ⬆️

... and 23 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Jackie-Jiang
Jackie-Jiang reviewed Sep 19, 2023
View reviewed changes
Copy link
Contributor

@Jackie-Jiang Jackie-Jiang left a comment

Choose a reason for hiding this comment

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

Seems we do have some scalar functions defined in pinot-core. Is it okay to not include them?

@xiangfu0
Copy link
Contributor Author

Seems we do have some scalar functions defined in pinot-core. Is it okay to not include them?

It's ok if we exclude entire pinot-core, as the scalar functions won't even be discovered.
The previous issue is that the client wants to register those scaler functions from pinot-core but won't be able to find the dependencies.

walterddr
walterddr reviewed Sep 19, 2023
View reviewed changes
pinot-core/src/main/java/org/apache/pinot/core/auth/BasicAuthUtils.java
@@ -64,21 +61,22 @@ private BasicAuthUtils() {
* @return list of BasicAuthPrincipals
*/
public static List<BasicAuthPrincipal> extractBasicAuthPrincipals(PinotConfiguration configuration, String prefix) {
String principalNames = configuration.getProperty(prefix);
Preconditions.checkArgument(StringUtils.isNotBlank(principalNames), "must provide principals");
String principalNames = configuration.getProperty(prefix);
Copy link
Contributor

Choose a reason for hiding this comment

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

does it make sense to also refactor the rest of the utils into common? putting them in core seems weird to me: is there any situation when the rest of the function util is not utilized together with the common one?

walterddr
walterddr approved these changes Sep 19, 2023
View reviewed changes
Copy link
Contributor

@walterddr walterddr left a comment

Choose a reason for hiding this comment

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

lgtm for this step as a minimum refactoring needed to get rid of dependency from client on core module.

@xiangfu0 xiangfu0 merged commit 60abbcc into apache:master Sep 19, 2023
23 checks passed
@xiangfu0 xiangfu0 deleted the remove-pinot-core-dependency-from-pinot-jdbc-client branch September 19, 2023 17:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@walterddr walterddr walterddr approved these changes

@KKcorps KKcorps Awaiting requested review from KKcorps

@apucher apucher Awaiting requested review from apucher

@Jackie-Jiang Jackie-Jiang Awaiting requested review from Jackie-Jiang

Assignees
No one assigned
Labels
dependencies Pull requests that update a dependency file pinot-client refactor
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@xiangfu0 @codecov-commenter @walterddr @Jackie-Jiang