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

making PinotFS an interface and create BasePinotFS #8063

Merged
merged 2 commits into from
Jan 25, 2022

Conversation

walterddr
Copy link
Contributor

@walterddr walterddr commented Jan 24, 2022

Description

PinotFS should be an interface instead of abstract base class. Creating a BasePinotFS to accommodate the logics that requires member variables.

Release Notes

PinotFS is no longer an abstract class, instead user can continue to extend from BasePinotFS or change to implement PinotFS.

@mcvsubbu
Copy link
Contributor

We can (and probably should) change this, but it will be an incompat change. please be sure to mark it as such in the release.

@walterddr
Copy link
Contributor Author

thanks for the feedback @mcvsubbu :-) yes I have the exact same concern and will do just that. will follow up @Jackie-Jiang 's comment too. here is the context #8064

@mcvsubbu mcvsubbu added the backward-incompat Referenced by PRs that introduce or fix backward compat issues label Jan 24, 2022
@walterddr walterddr marked this pull request as ready for review January 24, 2022 21:52
@codecov-commenter
Copy link

codecov-commenter commented Jan 24, 2022

Codecov Report

Merging #8063 (19df74c) into master (e7ea235) will increase coverage by 26.73%.
The diff coverage is 73.68%.

❗ Current head 19df74c differs from pull request most recent head 4c2217c. Consider uploading reports for the commit 4c2217c to get more accurate results
Impacted file tree graph

@@              Coverage Diff              @@
##             master    #8063       +/-   ##
=============================================
+ Coverage     37.93%   64.66%   +26.73%     
- Complexity       81     4261     +4180     
=============================================
  Files          1606     1562       -44     
  Lines         83405    81525     -1880     
  Branches      12455    12251      -204     
=============================================
+ Hits          31638    52719    +21081     
+ Misses        49311    25048    -24263     
- Partials       2456     3758     +1302     
Flag Coverage Δ
integration1 ?
integration2 ?
unittests1 67.87% <72.22%> (?)
unittests2 14.24% <5.26%> (-0.01%) ⬇️

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

Impacted Files Coverage Δ
.../java/org/apache/pinot/spi/filesystem/PinotFS.java 0.00% <ø> (ø)
...a/org/apache/pinot/spi/filesystem/BasePinotFS.java 70.58% <70.58%> (ø)
.../org/apache/pinot/plugin/filesystem/S3PinotFS.java 38.70% <100.00%> (ø)
.../org/apache/pinot/spi/filesystem/LocalPinotFS.java 89.79% <100.00%> (+89.79%) ⬆️
...a/org/apache/pinot/common/metrics/MinionMeter.java 0.00% <0.00%> (-100.00%) ⬇️
...g/apache/pinot/common/metrics/ControllerMeter.java 0.00% <0.00%> (-100.00%) ⬇️
.../apache/pinot/common/metrics/BrokerQueryPhase.java 0.00% <0.00%> (-100.00%) ⬇️
.../apache/pinot/common/metrics/MinionQueryPhase.java 0.00% <0.00%> (-100.00%) ⬇️
...ache/pinot/server/access/AccessControlFactory.java 0.00% <0.00%> (-100.00%) ⬇️
...he/pinot/common/messages/SegmentReloadMessage.java 0.00% <0.00%> (-100.00%) ⬇️
... and 1153 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e7ea235...4c2217c. Read the comment docs.

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.

LGTM

private static final Logger LOGGER = LoggerFactory.getLogger(BasePinotFS.class);

/**
* Moves the file or directory from the src to dst. Does not keep the original file. If the dst has parent directories
Copy link
Contributor

Choose a reason for hiding this comment

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

The javadoc is the same as the interface, suggest removing this javadoc block

@Jackie-Jiang Jackie-Jiang merged commit cc2f3fe into apache:master Jan 25, 2022
@walterddr walterddr deleted the pinot_fs_spi branch December 6, 2023 16:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backward-incompat Referenced by PRs that introduce or fix backward compat issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants