Jackie-Jiang commented on a change in pull request #8305:
URL: https://github.com/apache/pinot/pull/8305#discussion_r822093982
##########
File path:
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/ZookeeperResource.java
##########
@@ -154,11 +159,12 @@ public SuccessResponse putData(
}
ZNRecord znRecord;
try {
- znRecord = (ZNRecord)
_znRecordSerializer.deserialize(data.getBytes(Charsets.UTF_8));
+ znRecord = MAPPER.readValue(data.getBytes(Charsets.UTF_8),
ZNRecord.class);
Review comment:
```suggestion
znRecord = MAPPER.readValue(data, ZNRecord.class);
```
##########
File path:
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/ZookeeperResource.java
##########
@@ -84,17 +94,12 @@ public String getData(
ZNRecord znRecord = _pinotHelixResourceManager.readZKData(path);
if (znRecord != null) {
- byte[] serializeBytes = _znRecordSerializer.serialize(znRecord);
- if (GZipCompressionUtil.isCompressed(serializeBytes)) {
- try {
- serializeBytes = GZipCompressionUtil.uncompress(new
ByteArrayInputStream(serializeBytes));
- } catch (IOException e) {
- throw new RuntimeException(e);
- }
- }
+ byte[] serializeBytes = serialize(znRecord);
return new String(serializeBytes, StandardCharsets.UTF_8);
Review comment:
Remove `serialize()`
```suggestion
try {
return MAPPER.writeValueAsString(znRecords);
} catch (IOException e) {
throw new RuntimeException(e);
}
```
##########
File path:
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/ZookeeperResource.java
##########
@@ -44,29 +44,39 @@
import javax.ws.rs.core.Response;
import org.apache.commons.lang.StringUtils;
import org.apache.helix.ZNRecord;
-import org.apache.helix.manager.zk.ZNRecordSerializer;
-import org.apache.helix.util.GZipCompressionUtil;
import org.apache.pinot.controller.api.access.AccessType;
import org.apache.pinot.controller.api.access.Authenticate;
import
org.apache.pinot.controller.api.exception.ControllerApplicationException;
import org.apache.pinot.controller.helix.core.PinotHelixResourceManager;
import org.apache.pinot.spi.utils.JsonUtils;
import org.apache.zookeeper.data.Stat;
+import org.codehaus.jackson.map.DeserializationConfig;
+import org.codehaus.jackson.map.ObjectMapper;
+import org.codehaus.jackson.map.SerializationConfig;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@Api(tags = Constants.ZOOKEEPER)
@Path("/")
public class ZookeeperResource {
+ private static final Logger LOGGER =
LoggerFactory.getLogger(ZookeeperResource.class);
- public static final Logger LOGGER =
LoggerFactory.getLogger(ZookeeperResource.class);
+ // Helix uses codehaus.jackson.map.ObjectMapper, hence we can't use pinot
JsonUtils here.
+ private static final ObjectMapper MAPPER = new ObjectMapper();
+
+ static {
Review comment:
Let's add a note stating this is configured the same as
`ZNRecordSerializer`
##########
File path:
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/ZookeeperResource.java
##########
@@ -187,14 +193,48 @@ public String ls(
path = validateAndNormalizeZKPath(path, true);
- List<String> children = _pinotHelixResourceManager.getZKChildren(path);
+ List<String> childNames = _pinotHelixResourceManager.getZKChildNames(path);
try {
- return JsonUtils.objectToString(children);
+ return JsonUtils.objectToString(childNames);
} catch (JsonProcessingException e) {
throw new RuntimeException(e);
}
}
+ @GET
+ @Path("/zk/getChildren")
+ @Produces(MediaType.APPLICATION_JSON)
+ @ApiOperation(value = "Get all child znodes")
+ @ApiResponses(value = {
+ @ApiResponse(code = 200, message = "Success"),
+ @ApiResponse(code = 404, message = "ZK Path not found"),
+ @ApiResponse(code = 204, message = "No Content"),
+ @ApiResponse(code = 500, message = "Internal server error")
+ })
+ public String getChildren(
+ @ApiParam(value = "Zookeeper Path, must start with /", required = true)
@QueryParam("path") String path) {
+
+ path = validateAndNormalizeZKPath(path, true);
+
+ List<ZNRecord> znRecords = _pinotHelixResourceManager.getZKChildren(path);
+ if (znRecords != null) {
+ List<ZNRecord> nonNullRecords = new ArrayList<>();
Review comment:
(minor)
```suggestion
List<ZNRecord> nonNullRecords = new ArrayList<>(znRecords.size());
```
##########
File path:
pinot-controller/src/test/java/org/apache/pinot/controller/api/ZookeeperResourceTest.java
##########
@@ -19,16 +19,33 @@
package org.apache.pinot.controller.api;
import java.io.IOException;
+import java.nio.charset.StandardCharsets;
import java.util.HashMap;
+import java.util.List;
import java.util.Map;
+import org.apache.helix.ZNRecord;
import org.apache.pinot.common.utils.URIUtils;
import org.apache.pinot.controller.ControllerTestUtils;
+import org.codehaus.jackson.map.DeserializationConfig;
+import org.codehaus.jackson.map.ObjectMapper;
+import org.codehaus.jackson.map.SerializationConfig;
+import org.codehaus.jackson.type.TypeReference;
import org.testng.Assert;
import org.testng.annotations.BeforeClass;
import org.testng.annotations.Test;
public class ZookeeperResourceTest {
+ private static final ObjectMapper MAPPER = new ObjectMapper();
Review comment:
Use the one in `ZookeeperResource`
##########
File path:
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/ZookeeperResource.java
##########
@@ -44,29 +44,39 @@
import javax.ws.rs.core.Response;
import org.apache.commons.lang.StringUtils;
import org.apache.helix.ZNRecord;
-import org.apache.helix.manager.zk.ZNRecordSerializer;
-import org.apache.helix.util.GZipCompressionUtil;
import org.apache.pinot.controller.api.access.AccessType;
import org.apache.pinot.controller.api.access.Authenticate;
import
org.apache.pinot.controller.api.exception.ControllerApplicationException;
import org.apache.pinot.controller.helix.core.PinotHelixResourceManager;
import org.apache.pinot.spi.utils.JsonUtils;
import org.apache.zookeeper.data.Stat;
+import org.codehaus.jackson.map.DeserializationConfig;
+import org.codehaus.jackson.map.ObjectMapper;
+import org.codehaus.jackson.map.SerializationConfig;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@Api(tags = Constants.ZOOKEEPER)
@Path("/")
public class ZookeeperResource {
+ private static final Logger LOGGER =
LoggerFactory.getLogger(ZookeeperResource.class);
- public static final Logger LOGGER =
LoggerFactory.getLogger(ZookeeperResource.class);
+ // Helix uses codehaus.jackson.map.ObjectMapper, hence we can't use pinot
JsonUtils here.
+ private static final ObjectMapper MAPPER = new ObjectMapper();
Review comment:
Let's make it package private and annotate it with `VisibleForTesting`
--
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.
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
