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

Add a new table config field for peer segment download. #5478

Merged
merged 4 commits into from Jun 2, 2020

Conversation

chenboat
Copy link
Contributor

@chenboat chenboat commented Jun 1, 2020

Description

Add a new optional string field peerSegmentDownloadScheme to the SegmentsValidationAndRetentionConfig in the TableConfig. The value can be http or https.
The field will enable download of segments for both realtime and offline table segments from peer servers. In the beginning, only realtime table segments download will be supported. The design details can be found in this section of the cwiki doc.

@mcvsubbu

Upgrade Notes

Does this PR prevent a zero down-time upgrade? (Assume upgrade order: Controller, Broker, Server, Minion)

  • Yes (Please label as backward-incompat, and complete the section below on Release Notes)

Does this PR fix a zero-downtime upgrade introduced earlier?

  • Yes (Please label this as backward-incompat, and complete the section below on Release Notes)

Does this PR otherwise need attention when creating release notes? Things to consider:

  • New configuration options
  • Deprecation of configurations
  • Signature changes to public methods/interfaces
  • New plugins added or old plugins removed
  • Yes (Please label this PR as release-notes and complete the section on Release Notes)

Release Notes

Add a new optional string field peerSegmentDownloadScheme to the SegmentsValidationAndRetentionConfig in the TableConfig. The value can be http or https.
The field will enable download of segments for both realtime and offline table segments from peer servers. Currently, only realtime table segment download during LLC segment completion protocol will be supported. For more details, please refer to the cwiki design doc "By-passing deepstore requirement for realtime segment completion".

Documentation

If you have introduced a new feature or configuration, please add it to the documentation as well.
See https://docs.pinot.apache.org/developers/developers-and-contributors/update-document

Copy link
Contributor

@mcvsubbu mcvsubbu left a comment

Choose a reason for hiding this comment

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

Please add validation code ("Must be http or https") in TableConfigUtils.validate() thanks
Also, add more documentation on the new config variable (at least a pointer to the design doc).
Please label the PR marking it for release notes.
Make sure you add documentation on the config in gitbook (after you merge this PR)

@chenboat chenboat requested a review from mcvsubbu June 2, 2020 05:07
@chenboat
Copy link
Contributor Author

chenboat commented Jun 2, 2020

Please add validation code ("Must be http or https") in TableConfigUtils.validate() thanks
Also, add more documentation on the new config variable (at least a pointer to the design doc).
Please label the PR marking it for release notes.
Make sure you add documentation on the config in gitbook (after you merge this PR)

Done except that the last point about documentation.

Copy link
Contributor

@mcvsubbu mcvsubbu left a comment

Choose a reason for hiding this comment

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

Minor suggestions, otherwise looks good to me.

chenboat and others added 2 commits June 2, 2020 10:24
…g/TableConfigUtils.java

Co-authored-by: Subbu Subramaniam <mcvsubbu@users.noreply.github.com>
…g/TableConfigUtils.java

Co-authored-by: Subbu Subramaniam <mcvsubbu@users.noreply.github.com>
@mcvsubbu mcvsubbu merged commit 8a3eb43 into apache:master Jun 2, 2020
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.

None yet

2 participants