Skip to content

[WIP] Use lombok to simplify code#11756

Closed
zhtaoxiang wants to merge 4 commits into
apache:masterfrom
zhtaoxiang:use-lombok
Closed

[WIP] Use lombok to simplify code#11756
zhtaoxiang wants to merge 4 commits into
apache:masterfrom
zhtaoxiang:use-lombok

Conversation

@zhtaoxiang
Copy link
Copy Markdown
Contributor

@zhtaoxiang zhtaoxiang commented Oct 6, 2023

label: refactor

Use lombok in al pinot modules:

  1. replace getter methods with @getter
  2. replace setter methods with @setter
  3. replace constructors with @NoArgsConstructor and @AllArgsConstructor

There is no logic change.

Done modules:

  • pinot-broker
  • pinot-clients
  • pinot-common
  • pinot-compatibility-verifier
  • pinot-connectors
  • pinot-controller
  • pinot-core
  • pinot-distribution
  • pinot-integration-test-base
  • pinot-integration-tests
  • pinot-minion
  • pinot-perf
  • pinot-plugins
  • pinot-query-planner
  • pinot-query-runtime
  • pinot-segment-local
  • pinot-segment-spi
  • pinot-server
  • pinot-spi
  • pinot-tools

@zhtaoxiang zhtaoxiang changed the title Use lombok to simplify code [WIP] Use lombok to simplify code Oct 6, 2023
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Oct 6, 2023

Codecov Report

❌ Patch coverage is 63.07692% with 24 lines in your changes missing coverage. Please review.
✅ Project coverage is 63.07%. Comparing base (13c2901) to head (de1fa9e).
⚠️ Report is 4253 commits behind head on master.

Files with missing lines Patch % Lines
...ache/pinot/broker/queryquota/QueryQuotaEntity.java 36.36% 7 Missing ⚠️
...e/pinot/broker/broker/helix/BaseBrokerStarter.java 33.33% 6 Missing ⚠️
...c/main/java/org/apache/pinot/client/PinotMeta.java 0.00% 3 Missing ⚠️
...ient/JsonAsyncHttpPinotClientTransportFactory.java 66.66% 2 Missing ⚠️
...main/java/org/apache/pinot/client/PinotDriver.java 0.00% 2 Missing ⚠️
...broker/routing/instanceselector/SegmentStates.java 66.66% 1 Missing ⚠️
...main/java/org/apache/pinot/client/BrokerCache.java 0.00% 1 Missing ⚠️
.../main/java/org/apache/pinot/client/BrokerData.java 0.00% 1 Missing ⚠️
...g/apache/pinot/client/PinotConnectionMetaData.java 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master   #11756      +/-   ##
============================================
- Coverage     63.13%   63.07%   -0.07%     
- Complexity     1117     1118       +1     
============================================
  Files          2342     2342              
  Lines        125916   125807     -109     
  Branches      19370    19370              
============================================
- Hits          79501    79355     -146     
- Misses        40749    40798      +49     
+ Partials       5666     5654      -12     
Flag Coverage Δ
integration <0.01% <0.00%> (+<0.01%) ⬆️
integration1 <0.01% <0.00%> (+<0.01%) ⬆️
integration2 0.00% <0.00%> (ø)
java-11 63.04% <63.07%> (-0.03%) ⬇️
java-17 62.94% <63.07%> (-0.01%) ⬇️
java-20 50.07% <0.00%> (-12.92%) ⬇️
temurin 63.07% <63.07%> (-0.07%) ⬇️
unittests 63.07% <63.07%> (-0.07%) ⬇️
unittests1 67.22% <ø> (ø)
unittests2 14.37% <63.07%> (-0.11%) ⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@Jackie-Jiang Jackie-Jiang added cleanup Code cleanup or removal of dead code refactor Code restructuring without changing behavior labels Oct 6, 2023
Copy link
Copy Markdown
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.

Suggest adding it for POJO classes first. I feel it is less readable for regular classes

* pinot.broker.access.control.principals.user456.tables=stuff,lessImportantStuff
* </pre>
*/
@NoArgsConstructor
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't think this is needed because there is no other constructor. Same for other classes with single empty constructor

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

make sense. will clean up

@zhtaoxiang
Copy link
Copy Markdown
Contributor Author

Suggest adding it for POJO classes first. I feel it is less readable for regular classes

make sense

Copy link
Copy Markdown
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.

Mostly good. Please go over the classes again to fix the similar issues as the comments pointed out

throws SQLException {
_maxRows = rows;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

(minor) revert

@AllArgsConstructor
public class QueryQuotaEntity {

@Getter
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No need to keep exact match. For POJO, we can annotate the class

return _tablePartitionInfo;
}

@AllArgsConstructor(access = AccessLevel.PACKAGE)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

(minor) For private sub-class, no need to put access level

* Connections time out for AsyncHttpClient
*/
@AllArgsConstructor(access = AccessLevel.PRIVATE)
@Getter
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

(minor) not introduced in this PR, but let's reformat this file to fix the indentation

* <p>Please note that objects of this class will hold a reference to the given JsonNode object
* and that will only be released when the object is GC'ed.</p>
*/
@AllArgsConstructor(access = AccessLevel.PACKAGE)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Let's change it to public access

_preferTlsPort = preferTlsPort;
_zkClient = zkClient;
}
public ExternalViewReader(ZkClient zkClient) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

(minor) Add a space

@zhtaoxiang zhtaoxiang closed this Mar 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cleanup Code cleanup or removal of dead code refactor Code restructuring without changing behavior

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants