This is an automated email from the ASF dual-hosted git repository.
jackie pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/pinot.git
The following commit(s) were added to refs/heads/master by this push:
new 5eddb19d24 Merge ControllerSegmentUrlBuilder into
ControllerRequestURLBuilder (#8575)
5eddb19d24 is described below
commit 5eddb19d2486bb45ead9ae1b1fb31037ebe788a2
Author: Xiaotian (Jackie) Jiang <[email protected]>
AuthorDate: Fri Apr 22 13:23:42 2022 -0700
Merge ControllerSegmentUrlBuilder into ControllerRequestURLBuilder (#8575)
- Merge ControllerSegmentUrlBuilder into ControllerRequestURLBuilder
- Fix some missing encoding for segment name
---
.../controller/helix/ControllerRequestClient.java | 2 +-
.../helix/ControllerSegmentUrlBuilder.java | 42 ----------------------
.../pinot/controller/ControllerTestUtils.java | 23 +++++-------
.../pinot/controller/api/AccessControlTest.java | 7 ++--
.../pinot/controller/helix/ControllerTest.java | 2 --
.../tests/BasicAuthRealtimeIntegrationTest.java | 2 +-
.../tests/HybridClusterIntegrationTest.java | 26 +++++++-------
.../integration/tests/TlsIntegrationTest.java | 8 ++---
.../org/apache/pinot/spi/utils/StringUtil.java | 1 -
.../utils/builder/ControllerRequestURLBuilder.java | 40 ++++++++++++++-------
10 files changed, 55 insertions(+), 98 deletions(-)
diff --git
a/pinot-controller/src/main/java/org/apache/pinot/controller/helix/ControllerRequestClient.java
b/pinot-controller/src/main/java/org/apache/pinot/controller/helix/ControllerRequestClient.java
index ebb08ce54e..f189d4c2d7 100644
---
a/pinot-controller/src/main/java/org/apache/pinot/controller/helix/ControllerRequestClient.java
+++
b/pinot-controller/src/main/java/org/apache/pinot/controller/helix/ControllerRequestClient.java
@@ -163,7 +163,7 @@ public class ControllerRequestClient {
throws IOException {
try {
HttpClient.wrapAndThrowHttpException(_httpClient.sendDeleteRequest(new
URL(
- _controllerRequestURLBuilder.forSegmentDeleteAllAPI(tableName,
tableType.toString())).toURI()));
+ _controllerRequestURLBuilder.forSegmentDeleteAll(tableName,
tableType.toString())).toURI()));
} catch (HttpErrorStatusException | URISyntaxException e) {
throw new IOException(e);
}
diff --git
a/pinot-controller/src/main/java/org/apache/pinot/controller/helix/ControllerSegmentUrlBuilder.java
b/pinot-controller/src/main/java/org/apache/pinot/controller/helix/ControllerSegmentUrlBuilder.java
deleted file mode 100644
index 457693c00b..0000000000
---
a/pinot-controller/src/main/java/org/apache/pinot/controller/helix/ControllerSegmentUrlBuilder.java
+++ /dev/null
@@ -1,42 +0,0 @@
-/**
- * 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.controller.helix;
-
-import org.apache.commons.lang3.StringUtils;
-import org.apache.pinot.common.utils.URIUtils;
-
-public class ControllerSegmentUrlBuilder {
- private final String _baseUrl;
-
- private ControllerSegmentUrlBuilder(String baseUrl) {
- _baseUrl = StringUtils.chomp(baseUrl, "/");
- }
-
- public static ControllerSegmentUrlBuilder baseUrl(String baseUrl) {
- return new ControllerSegmentUrlBuilder(baseUrl);
- }
-
- public String forSegmentDeleteAPI(String tableName, String segmentName,
String tableType) {
- return URIUtils.getPath(_baseUrl, "segments", tableName,
URIUtils.encode(segmentName)) + "?type=" + tableType;
- }
-
- public String forSegmentDownload(String tableName, String segmentName) {
- return URIUtils.constructDownloadUrl(_baseUrl, tableName, segmentName);
- }
-}
diff --git
a/pinot-controller/src/test/java/org/apache/pinot/controller/ControllerTestUtils.java
b/pinot-controller/src/test/java/org/apache/pinot/controller/ControllerTestUtils.java
index 18dd081bf8..3187520352 100644
---
a/pinot-controller/src/test/java/org/apache/pinot/controller/ControllerTestUtils.java
+++
b/pinot-controller/src/test/java/org/apache/pinot/controller/ControllerTestUtils.java
@@ -57,7 +57,6 @@ import org.apache.pinot.common.utils.config.TagNameUtils;
import org.apache.pinot.common.utils.http.HttpClient;
import org.apache.pinot.controller.api.AccessControlTest;
import org.apache.pinot.controller.helix.ControllerRequestClient;
-import org.apache.pinot.controller.helix.ControllerSegmentUrlBuilder;
import org.apache.pinot.controller.helix.core.PinotHelixResourceManager;
import org.apache.pinot.spi.config.table.TableConfig;
import org.apache.pinot.spi.data.DateTimeFieldSpec;
@@ -104,7 +103,6 @@ public abstract class ControllerTestUtils {
protected static int _controllerPort;
protected static String _controllerBaseApiUrl;
protected static ControllerRequestURLBuilder _controllerRequestURLBuilder;
- protected static ControllerSegmentUrlBuilder _controllerSegmentUrlBuilder;
protected static HttpClient _httpClient = null;
protected static ControllerRequestClient _controllerRequestClient = null;
@@ -184,7 +182,6 @@ public abstract class ControllerTestUtils {
_controllerPort = Integer.parseInt(_controllerConfig.getControllerPort());
_controllerBaseApiUrl = "http://localhost:"; + _controllerPort;
_controllerRequestURLBuilder =
ControllerRequestURLBuilder.baseUrl(_controllerBaseApiUrl);
- _controllerSegmentUrlBuilder =
ControllerSegmentUrlBuilder.baseUrl(_controllerBaseApiUrl);
_controllerDataDir = _controllerConfig.getDataDir();
_controllerStarter = new ControllerStarter();
@@ -525,8 +522,8 @@ public abstract class ControllerTestUtils {
public static String sendGetRequest(String urlString)
throws IOException {
try {
- SimpleHttpResponse resp =
HttpClient.wrapAndThrowHttpException(getHttpClient().sendGetRequest(
- new URL(urlString).toURI()));
+ SimpleHttpResponse resp =
+
HttpClient.wrapAndThrowHttpException(getHttpClient().sendGetRequest(new
URL(urlString).toURI()));
return constructResponse(resp);
} catch (URISyntaxException | HttpErrorStatusException e) {
throw new IOException(e);
@@ -546,8 +543,8 @@ public abstract class ControllerTestUtils {
public static String sendPostRequest(String urlString, String payload,
Map<String, String> headers)
throws IOException {
try {
- SimpleHttpResponse resp =
HttpClient.wrapAndThrowHttpException(getHttpClient().sendJsonPostRequest(
- new URL(urlString).toURI(), payload, headers));
+ SimpleHttpResponse resp = HttpClient.wrapAndThrowHttpException(
+ getHttpClient().sendJsonPostRequest(new URL(urlString).toURI(),
payload, headers));
return constructResponse(resp);
} catch (URISyntaxException | HttpErrorStatusException e) {
throw new IOException(e);
@@ -567,8 +564,8 @@ public abstract class ControllerTestUtils {
public static String sendPutRequest(String urlString, String payload,
Map<String, String> headers)
throws IOException {
try {
- SimpleHttpResponse resp =
HttpClient.wrapAndThrowHttpException(getHttpClient().sendJsonPutRequest(
- new URL(urlString).toURI(), payload, headers));
+ SimpleHttpResponse resp = HttpClient.wrapAndThrowHttpException(
+ getHttpClient().sendJsonPutRequest(new URL(urlString).toURI(),
payload, headers));
return constructResponse(resp);
} catch (URISyntaxException | HttpErrorStatusException e) {
throw new IOException(e);
@@ -578,8 +575,8 @@ public abstract class ControllerTestUtils {
public static String sendDeleteRequest(String urlString)
throws IOException {
try {
- SimpleHttpResponse resp =
HttpClient.wrapAndThrowHttpException(getHttpClient().sendDeleteRequest(
- new URL(urlString).toURI()));
+ SimpleHttpResponse resp =
+
HttpClient.wrapAndThrowHttpException(getHttpClient().sendDeleteRequest(new
URL(urlString).toURI()));
return constructResponse(resp);
} catch (URISyntaxException | HttpErrorStatusException e) {
throw new IOException(e);
@@ -630,10 +627,6 @@ public abstract class ControllerTestUtils {
return _controllerRequestURLBuilder;
}
- public static ControllerSegmentUrlBuilder getControllerSegmentUrlBuilder() {
- return _controllerSegmentUrlBuilder;
- }
-
public static HelixAdmin getHelixAdmin() {
return _helixAdmin;
}
diff --git
a/pinot-controller/src/test/java/org/apache/pinot/controller/api/AccessControlTest.java
b/pinot-controller/src/test/java/org/apache/pinot/controller/api/AccessControlTest.java
index 00b10f10bd..0ea82923cf 100644
---
a/pinot-controller/src/test/java/org/apache/pinot/controller/api/AccessControlTest.java
+++
b/pinot-controller/src/test/java/org/apache/pinot/controller/api/AccessControlTest.java
@@ -41,18 +41,15 @@ public class AccessControlTest {
public void testAccessDenied() {
try {
ControllerTestUtils.sendGetRequest(
-
ControllerTestUtils.getControllerSegmentUrlBuilder().forSegmentDownload(TABLE_NAME,
"testSegment"));
+
ControllerTestUtils.getControllerRequestURLBuilder().forSegmentDownload(TABLE_NAME,
"testSegment"));
Assert.fail("Access not denied");
} catch (IOException e) {
Assert.assertTrue(e.getMessage().contains("Got error status code: 403
(Forbidden)"));
- return;
}
}
public static class DenyAllAccessFactory implements AccessControlFactory {
- private static final AccessControl DENY_ALL_ACCESS = (httpHeaders,
tableName) -> {
- return !tableName.equals(TABLE_NAME);
- };
+ private static final AccessControl DENY_ALL_ACCESS = (httpHeaders,
tableName) -> !tableName.equals(TABLE_NAME);
@Override
public AccessControl create() {
diff --git
a/pinot-controller/src/test/java/org/apache/pinot/controller/helix/ControllerTest.java
b/pinot-controller/src/test/java/org/apache/pinot/controller/helix/ControllerTest.java
index 26135fa248..bde4eb3357 100644
---
a/pinot-controller/src/test/java/org/apache/pinot/controller/helix/ControllerTest.java
+++
b/pinot-controller/src/test/java/org/apache/pinot/controller/helix/ControllerTest.java
@@ -87,7 +87,6 @@ public abstract class ControllerTest {
protected static int _controllerPort;
protected static String _controllerBaseApiUrl;
protected static ControllerRequestURLBuilder _controllerRequestURLBuilder;
- protected static ControllerSegmentUrlBuilder _controllerSegmentUrlBuilder;
protected static HttpClient _httpClient = null;
protected static ControllerRequestClient _controllerRequestClient = null;
@@ -189,7 +188,6 @@ public abstract class ControllerTest {
_controllerBaseApiUrl = controllerScheme + "://localhost:" +
_controllerPort;
_controllerRequestURLBuilder =
ControllerRequestURLBuilder.baseUrl(_controllerBaseApiUrl);
- _controllerSegmentUrlBuilder =
ControllerSegmentUrlBuilder.baseUrl(_controllerBaseApiUrl);
_controllerDataDir = config.getDataDir();
_controllerStarter = getControllerStarter();
diff --git
a/pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/BasicAuthRealtimeIntegrationTest.java
b/pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/BasicAuthRealtimeIntegrationTest.java
index 66144cb3f8..baa9972644 100644
---
a/pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/BasicAuthRealtimeIntegrationTest.java
+++
b/pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/BasicAuthRealtimeIntegrationTest.java
@@ -192,7 +192,7 @@ public class BasicAuthRealtimeIntegrationTest extends
BaseClusterIntegrationTest
for (int i = 0; i < offlineSegments.size(); i++) {
String segment = offlineSegments.get(i).asText();
Assert.assertTrue(
-
sendGetRequest(_controllerSegmentUrlBuilder.forSegmentDownload(getTableName(),
segment), AUTH_HEADER).length()
+
sendGetRequest(_controllerRequestURLBuilder.forSegmentDownload(getTableName(),
segment), AUTH_HEADER).length()
> 200000); // download segment
}
}
diff --git
a/pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/HybridClusterIntegrationTest.java
b/pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/HybridClusterIntegrationTest.java
index 65d53275d6..6c23591b75 100644
---
a/pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/HybridClusterIntegrationTest.java
+++
b/pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/HybridClusterIntegrationTest.java
@@ -83,8 +83,8 @@ public class HybridClusterIntegrationTest extends
BaseClusterIntegrationTestSet
addTableConfig(createRealtimeTableConfig(realtimeAvroFiles.get(0)));
// Create and upload segments
- ClusterIntegrationTestUtils
- .buildSegmentsFromAvro(offlineAvroFiles, offlineTableConfig, schema,
0, _segmentDir, _tarDir);
+ ClusterIntegrationTestUtils.buildSegmentsFromAvro(offlineAvroFiles,
offlineTableConfig, schema, 0, _segmentDir,
+ _tarDir);
uploadSegments(getTableName(), _tarDir);
// Push data into Kafka
@@ -123,18 +123,16 @@ public class HybridClusterIntegrationTest extends
BaseClusterIntegrationTestSet
@Test
public void testSegmentMetadataApi()
throws Exception {
- {
- String jsonOutputStr =
sendGetRequest(_controllerRequestURLBuilder.forSegmentsMetadataFromServer(getTableName()));
- JsonNode tableSegmentsMetadata =
JsonUtils.stringToJsonNode(jsonOutputStr);
- Assert.assertEquals(tableSegmentsMetadata.size(), 8);
-
- JsonNode segmentMetadataFromAllEndpoint =
tableSegmentsMetadata.elements().next();
- String segmentName =
URLEncoder.encode(segmentMetadataFromAllEndpoint.get("segmentName").asText(),
"UTF-8");
- jsonOutputStr =
sendGetRequest(_controllerRequestURLBuilder.forSegmentMetadata(getTableName(),
segmentName));
- JsonNode segmentMetadataFromDirectEndpoint =
JsonUtils.stringToJsonNode(jsonOutputStr);
- Assert.assertEquals(segmentMetadataFromAllEndpoint.get("totalDocs"),
- segmentMetadataFromDirectEndpoint.get("segment.total.docs"));
- }
+ String jsonOutputStr =
sendGetRequest(_controllerRequestURLBuilder.forSegmentsMetadataFromServer(getTableName()));
+ JsonNode tableSegmentsMetadata = JsonUtils.stringToJsonNode(jsonOutputStr);
+ Assert.assertEquals(tableSegmentsMetadata.size(), 8);
+
+ JsonNode segmentMetadataFromAllEndpoint =
tableSegmentsMetadata.elements().next();
+ String segmentName =
segmentMetadataFromAllEndpoint.get("segmentName").asText();
+ jsonOutputStr =
sendGetRequest(_controllerRequestURLBuilder.forSegmentMetadata(getTableName(),
segmentName));
+ JsonNode segmentMetadataFromDirectEndpoint =
JsonUtils.stringToJsonNode(jsonOutputStr);
+ Assert.assertEquals(segmentMetadataFromAllEndpoint.get("totalDocs"),
+ segmentMetadataFromDirectEndpoint.get("segment.total.docs"));
}
@Test
diff --git
a/pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/TlsIntegrationTest.java
b/pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/TlsIntegrationTest.java
index 6615d968f9..bbc86c2e5d 100644
---
a/pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/TlsIntegrationTest.java
+++
b/pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/TlsIntegrationTest.java
@@ -283,10 +283,8 @@ public class TlsIntegrationTest extends
BaseClusterIntegrationTest {
public void testUpdatedBrokerTlsPort() {
List<InstanceConfig> instanceConfigs =
HelixHelper.getInstanceConfigs(_helixManager);
- List<ExtraInstanceConfig> securedInstances =
- instanceConfigs.stream().map(ExtraInstanceConfig::new)
- .filter(pinotInstanceConfig -> pinotInstanceConfig.getTlsPort() !=
null)
- .collect(Collectors.toList());
+ List<ExtraInstanceConfig> securedInstances =
instanceConfigs.stream().map(ExtraInstanceConfig::new)
+ .filter(pinotInstanceConfig -> pinotInstanceConfig.getTlsPort() !=
null).collect(Collectors.toList());
Assert.assertFalse(securedInstances.isEmpty());
}
@@ -493,7 +491,7 @@ public class TlsIntegrationTest extends
BaseClusterIntegrationTest {
for (int i = 0; i < offlineSegments.size(); i++) {
String segment = offlineSegments.get(i).asText();
Assert.assertTrue(
-
sendGetRequest(_controllerSegmentUrlBuilder.forSegmentDownload(getTableName(),
segment), AUTH_HEADER).length()
+
sendGetRequest(_controllerRequestURLBuilder.forSegmentDownload(getTableName(),
segment), AUTH_HEADER).length()
> 200000); // download segment
}
}
diff --git a/pinot-spi/src/main/java/org/apache/pinot/spi/utils/StringUtil.java
b/pinot-spi/src/main/java/org/apache/pinot/spi/utils/StringUtil.java
index be8327b257..6939a50b13 100644
--- a/pinot-spi/src/main/java/org/apache/pinot/spi/utils/StringUtil.java
+++ b/pinot-spi/src/main/java/org/apache/pinot/spi/utils/StringUtil.java
@@ -21,7 +21,6 @@ package org.apache.pinot.spi.utils;
import org.apache.commons.lang.StringUtils;
-// TODO: Use pinot-spi StringUtils instead
public class StringUtil {
private StringUtil() {
}
diff --git
a/pinot-spi/src/main/java/org/apache/pinot/spi/utils/builder/ControllerRequestURLBuilder.java
b/pinot-spi/src/main/java/org/apache/pinot/spi/utils/builder/ControllerRequestURLBuilder.java
index dd7c032073..e57b7a14d2 100644
---
a/pinot-spi/src/main/java/org/apache/pinot/spi/utils/builder/ControllerRequestURLBuilder.java
+++
b/pinot-spi/src/main/java/org/apache/pinot/spi/utils/builder/ControllerRequestURLBuilder.java
@@ -36,7 +36,12 @@ public class ControllerRequestURLBuilder {
private final String _baseUrl;
private ControllerRequestURLBuilder(String baseUrl) {
- _baseUrl = StringUtils.chomp(baseUrl, "/");
+ int length = baseUrl.length();
+ if (baseUrl.charAt(length - 1) == '/') {
+ _baseUrl = baseUrl.substring(0, length - 1);
+ } else {
+ _baseUrl = baseUrl;
+ }
}
public static ControllerRequestURLBuilder baseUrl(String baseUrl) {
@@ -189,17 +194,10 @@ public class ControllerRequestURLBuilder {
}
public String forTableReload(String tableName, TableType tableType, boolean
forceDownload) {
- String query = String.format("reload?forceDownload=%s&type=%s",
forceDownload, tableType.name());
+ String query = String.format("reload?type=%s&forceDownload=%s",
tableType.name(), forceDownload);
return StringUtil.join("/", _baseUrl, "segments", tableName, query);
}
- public String forSegmentReload(String tableName, String segmentName, boolean
forceDownload)
- throws UnsupportedEncodingException {
- String query = "reload?forceDownload=" + forceDownload;
- String segName = URLEncoder.encode(segmentName,
StandardCharsets.UTF_8.toString());
- return StringUtil.join("/", _baseUrl, "segments", tableName, segName,
query);
- }
-
public String forTableSize(String tableName) {
return StringUtil.join("/", _baseUrl, "tables", tableName, "size");
}
@@ -280,11 +278,20 @@ public class ControllerRequestURLBuilder {
return StringUtil.join("/", _baseUrl, "tableConfigs", "validate");
}
+ public String forSegmentReload(String tableName, String segmentName, boolean
forceDownload) {
+ return StringUtil.join("/", _baseUrl, "segments", tableName,
encode(segmentName),
+ "reload?forceDownload=" + forceDownload);
+ }
+
+ public String forSegmentDownload(String tableName, String segmentName) {
+ return StringUtil.join("/", _baseUrl, "segments", tableName,
encode(segmentName));
+ }
+
public String forSegmentDelete(String tableName, String segmentName) {
- return StringUtil.join("/", _baseUrl, "segments", tableName, segmentName);
+ return StringUtil.join("/", _baseUrl, "segments", tableName,
encode(segmentName));
}
- public String forSegmentDeleteAllAPI(String tableName, String tableType) {
+ public String forSegmentDeleteAll(String tableName, String tableType) {
return StringUtil.join("/", _baseUrl, "segments", tableName + "?type=" +
tableType);
}
@@ -305,7 +312,7 @@ public class ControllerRequestURLBuilder {
}
public String forSegmentMetadata(String tableName, String segmentName) {
- return StringUtil.join("/", _baseUrl, "segments", tableName, segmentName,
"metadata");
+ return StringUtil.join("/", _baseUrl, "segments", tableName,
encode(segmentName), "metadata");
}
public String forListAllCrcInformationForTable(String tableName) {
@@ -409,4 +416,13 @@ public class ControllerRequestURLBuilder {
public String forZkGetChildren(String path) {
return StringUtil.join("/", _baseUrl, "zk/getChildren", "?path=" + path);
}
+
+ private static String encode(String s) {
+ try {
+ return URLEncoder.encode(s, "UTF-8");
+ } catch (Exception e) {
+ // Should never happen
+ throw new RuntimeException(e);
+ }
+ }
}
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
