You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by GitBox <gi...@apache.org> on 2020/07/13 23:06:25 UTC
[GitHub] [incubator-pinot] chenboat opened a new pull request #5700: [Deepstore by-passing]Introduce a subclasses SplitSegmentCommitter which will proceeds to commit even if the segment upload fails.
chenboat opened a new pull request #5700:
URL: https://github.com/apache/incubator-pinot/pull/5700
## Description
Introduce a subclasses SplitSegmentCommitter which will proceeds to commit even if the segment upload fails. Design [doc](https://cwiki.apache.org/confluence/display/PINOT/By-passing+deep-store+requirement+for+Realtime+segment+completion).
## Upgrade Notes
Does this PR prevent a zero down-time upgrade? (Assume upgrade order: Controller, Broker, Server, Minion)
* [ ] Yes (Please label as **<code>backward-incompat</code>**, and complete the section below on Release Notes)
Does this PR fix a zero-downtime upgrade introduced earlier?
* [ ] Yes (Please label this as **<code>backward-incompat</code>**, 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 **<code>release-notes</code>** and complete the section on Release Notes)
## Release Notes
If you have tagged this as either backward-incompat or release-notes,
you MUST add text here that you would like to see appear in release notes of the
next release.
If you have a series of commits adding or enabling a feature, then
add this section only in final commit that marks the feature completed.
Refer to earlier release notes to see examples of text
## 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
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] chenboat commented on a change in pull request #5700: [Deepstore by-passing]Introduce a subclasses SplitSegmentCommitter which will proceeds to commit even if the segment upload fails.
Posted by GitBox <gi...@apache.org>.
chenboat commented on a change in pull request #5700:
URL: https://github.com/apache/incubator-pinot/pull/5700#discussion_r459631008
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/data/manager/realtime/PeerSchemeSplitSegmentCommitter.java
##########
@@ -0,0 +1,51 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.core.data.manager.realtime;
+
+import java.io.File;
+import java.net.URI;
+import org.apache.pinot.common.protocols.SegmentCompletionProtocol;
+import org.apache.pinot.common.utils.CommonConstants;
+import org.apache.pinot.common.utils.LLCSegmentName;
+import org.apache.pinot.common.utils.StringUtil;
+import org.apache.pinot.server.realtime.ServerSegmentCompletionProtocolHandler;
+import org.slf4j.Logger;
+
+
+public class PeerSchemeSplitSegmentCommitter extends SplitSegmentCommitter {
+ public PeerSchemeSplitSegmentCommitter(Logger segmentLogger, ServerSegmentCompletionProtocolHandler protocolHandler,
+ SegmentCompletionProtocol.Request.Params params, SegmentUploader segmentUploader) {
+ super(segmentLogger, protocolHandler, params, segmentUploader);
+ }
+
+ // Always return true even if the segment upload fails and return null uri.
+ // If the segment upload fails, put peer:///segment_name in the segment location to notify the controller it is a
+ // peer download scheme.
+ protected boolean uploadSegment(File segmentTarFile, SegmentUploader segmentUploader,
Review comment:
returning a URL string is better imo. all the url construction has been done by the underlying segment uploader class.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] chenboat commented on a change in pull request #5700: [Deepstore by-passing]Introduce a subclasses SplitSegmentCommitter which will proceeds to commit even if the segment upload fails.
Posted by GitBox <gi...@apache.org>.
chenboat commented on a change in pull request #5700:
URL: https://github.com/apache/incubator-pinot/pull/5700#discussion_r459690008
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/CommonConstants.java
##########
@@ -187,6 +187,7 @@
"pinot.server.instance.realtime.alloc.offheap.direct";
public static final String PREFIX_OF_CONFIG_OF_PINOT_FS_FACTORY = "pinot.server.storage.factory";
public static final String PREFIX_OF_CONFIG_OF_PINOT_CRYPTER = "pinot.server.crypter";
+ public static final String CONFIG_OF_SEGMENT_STORE_URI= "pinot.server.instance.segment.store.uri";
Review comment:
good catch. done.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] mcvsubbu commented on a change in pull request #5700: [Deepstore by-passing]Introduce a subclasses SplitSegmentCommitter which will proceeds to commit even if the segment upload fails.
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on a change in pull request #5700:
URL: https://github.com/apache/incubator-pinot/pull/5700#discussion_r454706307
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/data/manager/realtime/LLRealtimeSegmentDataManager.java
##########
@@ -846,16 +846,22 @@ protected boolean commitSegment(String controllerVipUrl, boolean isSplitCommit)
if (isSplitCommit) {
// TODO: make segment uploader used in the segment committer configurable.
SegmentUploader segmentUploader;
- try {
- segmentUploader =
- new Server2ControllerSegmentUploader(segmentLogger, _protocolHandler.getFileUploadDownloadClient(),
- _protocolHandler.getSegmentCommitUploadURL(params, controllerVipUrl), _segmentNameStr,
- ServerSegmentCompletionProtocolHandler.getSegmentUploadRequestTimeoutMs(), _serverMetrics);
- } catch (URISyntaxException e) {
- segmentLogger.error("Segment commit upload url error: ", e);
- return SegmentCompletionProtocol.RESP_NOT_SENT;
+ if (this._tableConfig.getValidationConfig().getPeerSegmentDownloadScheme() != null) {
Review comment:
Move this logic into the SegmentCommitterFactory class. Instantiate the class with `tableConfig`
Change the factory to have a single call to create committer, with `params` and `isSplit`. The uploader can be constructed inside the factory (in the split case).
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/data/manager/realtime/PeerSchemeSplitSegmentCommitter.java
##########
@@ -0,0 +1,48 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.core.data.manager.realtime;
+
+import java.io.File;
+import java.net.URI;
+import org.apache.pinot.common.protocols.SegmentCompletionProtocol;
+import org.apache.pinot.common.utils.LLCSegmentName;
+import org.apache.pinot.server.realtime.ServerSegmentCompletionProtocolHandler;
+import org.slf4j.Logger;
+
+
+public class PeerSchemeSplitSegmentCommitter extends SplitSegmentCommitter {
+ public PeerSchemeSplitSegmentCommitter(Logger segmentLogger, ServerSegmentCompletionProtocolHandler protocolHandler,
+ SegmentCompletionProtocol.Request.Params params, SegmentUploader segmentUploader) {
+ super(segmentLogger, protocolHandler, params, segmentUploader);
+ }
+
+ // Always return true even if the segment upload fails and return null uri.
+ // If the segment upload fails, put peer:///segment_name in the segment location to notify the controller it is a
+ // peer download scheme.
+ 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());
+ } else {
+ params.withSegmentLocation("peer:///" + params.getSegmentName());
Review comment:
Please put "peer" as a scheme in CommonConstants and refer to it everywhere. Please change PinotLLCRealtimeSegmentManager, and the tests SegmentCompletionTest, and PinotLLCRealtimeSegmentManagerTest
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] mcvsubbu commented on a change in pull request #5700: [Deepstore by-passing]Introduce a subclasses SplitSegmentCommitter which will proceeds to commit even if the segment upload fails.
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on a change in pull request #5700:
URL: https://github.com/apache/incubator-pinot/pull/5700#discussion_r459096952
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/data/manager/realtime/PeerSchemeSplitSegmentCommitter.java
##########
@@ -0,0 +1,51 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.core.data.manager.realtime;
+
+import java.io.File;
+import java.net.URI;
+import org.apache.pinot.common.protocols.SegmentCompletionProtocol;
+import org.apache.pinot.common.utils.CommonConstants;
+import org.apache.pinot.common.utils.LLCSegmentName;
+import org.apache.pinot.common.utils.StringUtil;
+import org.apache.pinot.server.realtime.ServerSegmentCompletionProtocolHandler;
+import org.slf4j.Logger;
+
+
+public class PeerSchemeSplitSegmentCommitter extends SplitSegmentCommitter {
+ public PeerSchemeSplitSegmentCommitter(Logger segmentLogger, ServerSegmentCompletionProtocolHandler protocolHandler,
+ SegmentCompletionProtocol.Request.Params params, SegmentUploader segmentUploader) {
+ super(segmentLogger, protocolHandler, params, segmentUploader);
+ }
+
+ // Always return true even if the segment upload fails and return null uri.
+ // If the segment upload fails, put peer:///segment_name in the segment location to notify the controller it is a
+ // peer download scheme.
+ protected boolean uploadSegment(File segmentTarFile, SegmentUploader segmentUploader,
Review comment:
Does returning URL here make sense? (null if failed)
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] mcvsubbu commented on a change in pull request #5700: [Deepstore by-passing]Introduce a subclasses SplitSegmentCommitter which will proceeds to commit even if the segment upload fails.
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] chenboat commented on a change in pull request #5700: [Deepstore by-passing]Introduce a subclasses SplitSegmentCommitter which will proceeds to commit even if the segment upload fails.
Posted by GitBox <gi...@apache.org>.
chenboat commented on a change in pull request #5700:
URL: https://github.com/apache/incubator-pinot/pull/5700#discussion_r458391255
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/data/manager/realtime/LLRealtimeSegmentDataManager.java
##########
@@ -846,16 +846,22 @@ protected boolean commitSegment(String controllerVipUrl, boolean isSplitCommit)
if (isSplitCommit) {
// TODO: make segment uploader used in the segment committer configurable.
SegmentUploader segmentUploader;
- try {
- segmentUploader =
- new Server2ControllerSegmentUploader(segmentLogger, _protocolHandler.getFileUploadDownloadClient(),
- _protocolHandler.getSegmentCommitUploadURL(params, controllerVipUrl), _segmentNameStr,
- ServerSegmentCompletionProtocolHandler.getSegmentUploadRequestTimeoutMs(), _serverMetrics);
- } catch (URISyntaxException e) {
- segmentLogger.error("Segment commit upload url error: ", e);
- return SegmentCompletionProtocol.RESP_NOT_SENT;
+ if (this._tableConfig.getValidationConfig().getPeerSegmentDownloadScheme() != null) {
Review comment:
Done. thanks for the suggestion.. Much cleaner codes now.
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/data/manager/realtime/PeerSchemeSplitSegmentCommitter.java
##########
@@ -0,0 +1,48 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.core.data.manager.realtime;
+
+import java.io.File;
+import java.net.URI;
+import org.apache.pinot.common.protocols.SegmentCompletionProtocol;
+import org.apache.pinot.common.utils.LLCSegmentName;
+import org.apache.pinot.server.realtime.ServerSegmentCompletionProtocolHandler;
+import org.slf4j.Logger;
+
+
+public class PeerSchemeSplitSegmentCommitter extends SplitSegmentCommitter {
+ public PeerSchemeSplitSegmentCommitter(Logger segmentLogger, ServerSegmentCompletionProtocolHandler protocolHandler,
+ SegmentCompletionProtocol.Request.Params params, SegmentUploader segmentUploader) {
+ super(segmentLogger, protocolHandler, params, segmentUploader);
+ }
+
+ // Always return true even if the segment upload fails and return null uri.
+ // If the segment upload fails, put peer:///segment_name in the segment location to notify the controller it is a
+ // peer download scheme.
+ 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());
+ } else {
+ params.withSegmentLocation("peer:///" + params.getSegmentName());
Review comment:
Done.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] chenboat commented on a change in pull request #5700: [Deepstore by-passing]Introduce a subclasses SplitSegmentCommitter which will proceeds to commit even if the segment upload fails.
Posted by GitBox <gi...@apache.org>.
chenboat commented on a change in pull request #5700:
URL: https://github.com/apache/incubator-pinot/pull/5700#discussion_r459670728
##########
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:
done.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] mcvsubbu commented on a change in pull request #5700: [Deepstore by-passing]Introduce a subclasses SplitSegmentCommitter which will proceeds to commit even if the segment upload fails.
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on a change in pull request #5700:
URL: https://github.com/apache/incubator-pinot/pull/5700#discussion_r459633576
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/CommonConstants.java
##########
@@ -187,6 +187,7 @@
"pinot.server.instance.realtime.alloc.offheap.direct";
public static final String PREFIX_OF_CONFIG_OF_PINOT_FS_FACTORY = "pinot.server.storage.factory";
public static final String PREFIX_OF_CONFIG_OF_PINOT_CRYPTER = "pinot.server.crypter";
+ public static final String CONFIG_OF_SEGMENT_STORE_URI= "pinot.server.instance.segment.store.uri";
Review comment:
Unused, please remove
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] chenboat commented on a change in pull request #5700: [Deepstore by-passing]Introduce a subclasses SplitSegmentCommitter which will proceeds to commit even if the segment upload fails.
Posted by GitBox <gi...@apache.org>.
chenboat commented on a change in pull request #5700:
URL: https://github.com/apache/incubator-pinot/pull/5700#discussion_r459682187
##########
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:
Yes. The current way is not done cleanly. I will think more about this and fix it in a follow up discussion and PR.
What we talk about here is how to construct different types of split committers as well as segment loaders. server level config is a viable option. For the controller response option, the downside is that it will affect all servers behavior in a cluster.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] chenboat commented on a change in pull request #5700: [Deepstore by-passing]Introduce a subclasses SplitSegmentCommitter which will proceeds to commit even if the segment upload fails.
Posted by GitBox <gi...@apache.org>.
chenboat commented on a change in pull request #5700:
URL: https://github.com/apache/incubator-pinot/pull/5700#discussion_r459630215
##########
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:
done together with change with the method return type.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] chenboat commented on a change in pull request #5700: [Deepstore by-passing]Introduce a subclasses SplitSegmentCommitter which will proceeds to commit even if the segment upload fails.
Posted by GitBox <gi...@apache.org>.
chenboat commented on a change in pull request #5700:
URL: https://github.com/apache/incubator-pinot/pull/5700#discussion_r459670249
##########
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:
Done.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] mcvsubbu merged pull request #5700: [Deepstore by-passing]Introduce a subclasses SplitSegmentCommitter which will proceeds to commit even if the segment upload fails.
Posted by GitBox <gi...@apache.org>.
mcvsubbu merged pull request #5700:
URL: https://github.com/apache/incubator-pinot/pull/5700
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] mcvsubbu commented on pull request #5700: [Deepstore by-passing]Introduce a subclasses SplitSegmentCommitter which will proceeds to commit even if the segment upload fails.
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on pull request #5700:
URL: https://github.com/apache/incubator-pinot/pull/5700#issuecomment-663165120
@chenboat please add some tests in another follow-up commit. Here is what I would suggest:
1. You can start a thread that behaves like deepstore, failing the commit. Start another thread, that starts only the committer, mocking everything else. Start a server fake server thread that in externalview seem online for the segment. Assert that the server gets a request for download, and assert the segment commit passes. This verifies only the server side.
2. Controller verificaton can be done independently, I think. All that we need to verify is that when a segment is committed with "PEER" in the URI, the metadata for segment URI is null. You can probably do this by starting a controller, mocking a realtime segment, and posting a consumed followed by commit call using http.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org