mcvsubbu commented on a change in pull request #5700:
URL: https://github.com/apache/incubator-pinot/pull/5700#discussion_r459095899
##########
File path:
pinot-core/src/main/java/org/apache/pinot/core/data/manager/realtime/SplitSegmentCommitter.java
##########
@@ -70,4 +68,15 @@ public SplitSegmentCommitter(Logger segmentLogger,
ServerSegmentCompletionProtoc
}
return commitEndResponse;
}
+
+ // Return false iff the segment upload fails.
+ protected boolean uploadSegment(File segmentTarFile, SegmentUploader
segmentUploader,
+ SegmentCompletionProtocol.Request.Params params) {
+ URI segmentLocation = segmentUploader.uploadSegment(segmentTarFile, new
LLCSegmentName(params.getSegmentName()));
+ if (segmentLocation != null) {
+ params.withSegmentLocation(segmentLocation.toString());
Review comment:
Why are we changing request parameters while indicating response? Can we
remove this?
##########
File path:
pinot-core/src/main/java/org/apache/pinot/core/data/manager/realtime/SegmentCommitterFactory.java
##########
@@ -29,18 +33,35 @@
public class SegmentCommitterFactory {
private static Logger LOGGER;
private final ServerSegmentCompletionProtocolHandler _protocolHandler;
+ private final TableConfig _tableConfig;
+ private final ServerMetrics _serverMetrics;
+ private final IndexLoadingConfig _indexLoadingConfig;
- public SegmentCommitterFactory(Logger segmentLogger,
ServerSegmentCompletionProtocolHandler protocolHandler) {
+ public SegmentCommitterFactory(Logger segmentLogger,
ServerSegmentCompletionProtocolHandler protocolHandler,
+ TableConfig tableConfig, IndexLoadingConfig indexLoadingConfig,
ServerMetrics serverMetrics) {
LOGGER = segmentLogger;
_protocolHandler = protocolHandler;
+ _tableConfig = tableConfig;
+ _indexLoadingConfig = indexLoadingConfig;
+ _serverMetrics = serverMetrics;
}
- public SegmentCommitter
createSplitSegmentCommitter(SegmentCompletionProtocol.Request.Params params,
- SegmentUploader segmentUploader) {
- return new SplitSegmentCommitter(LOGGER, _protocolHandler, params,
segmentUploader);
- }
+ public SegmentCommitter createSegmentCommitter(boolean isSplitCommit,
SegmentCompletionProtocol.Request.Params params,
+ String controllerVipUrl) throws URISyntaxException {
+ if (!isSplitCommit) {
+ return new DefaultSegmentCommitter(LOGGER, _protocolHandler, params);
+ }
+ SegmentUploader segmentUploader;
+ if (_tableConfig.getValidationConfig().getPeerSegmentDownloadScheme() !=
null) {
Review comment:
Can you add a TODO here? It looks odd that we upload to PinotFS if peer
download scheme is set.
We should have an independent config of where to upload segments. I am not
sure if this config should be:
(a) A table config (b) A server level config (c) Upload to whatever the
controller asks us to do (so then becomes a cluster or controller level config).
I like (c) ,and definitely do not want (a) for this.
I would like to hear from you and @npawar and others if they have an opinion.
For now, this is ok since you need to move along with your implementation.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
