You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@helix.apache.org by GitBox <gi...@apache.org> on 2021/02/28 21:22:40 UTC
[GitHub] [helix] pkuwm opened a new pull request #1659: Compatible support for ZNRecord to work with Jackson 1 mapper
pkuwm opened a new pull request #1659:
URL: https://github.com/apache/helix/pull/1659
### Issues
- [x] My PR addresses the following Helix issues and references them in the PR description:
Fixes #1655
### Description
- [x] Here are some details about my PR, including screenshots of any UI changes:
Jackson was upgraded in helix from 1.x to 2.x (https://github.com/apache/helix/pull/1293). Jackson 1.x does not support Jackson 2.x annotations.
This PR rolls back Jackson 2 annotations to Jackson 1, and adds a custom `CodehausJacksonAnnotationIntrospector` to make Jackson 2 work with Jackson 1 annotations. The custom "CodehausJacksonAnnotationIntrospector" is based on Jackson's recommendation on GitHub doc: https://github.com/FasterXML/jackson-annotations,
Backwards compatibility: You can make Jackson 2 use Jackson 1 annotations with [jackson-legacy-introspector](https://github.com/Laures/jackson-legacy-introspector)
The introspector is simplified and customized to ONLY work with Jackson 1 annotations in helix. It is implemented for backward compatibility, and should be deprecated once all Jackson 1 annotations are upgrade/removed in Helix.
Classes that roll back to use Jackson 1 annotations:
- `ZNRecord`
- `SessionAwareZNRecord`
### Tests
- [x] The following tests are written for this issue:
- `testCodehausJacksonSerializer`
- The following is the result of the "mvn test" command on the appropriate module:
zookeeper-api
```
[INFO] Tests run: 53, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 67.799 s - in TestSuite
[INFO]
[INFO] Results:
[INFO]
[INFO] Tests run: 53, Failures: 0, Errors: 0, Skipped: 0
[INFO]
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 01:11 min
[INFO] Finished at: 2021-02-26T17:56:52-08:00
[INFO] ------------------------------------------------------------------------
```
helix-core
```
```
helix-rest
```
[INFO] Tests run: 171, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 93.342 s - in TestSuite
[INFO]
[INFO] Results:
[INFO]
[INFO] Tests run: 171, Failures: 0, Errors: 0, Skipped: 0
[INFO]
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 01:38 min
[INFO] Finished at: 2021-02-27T21:08:48-08:00
[INFO] ------------------------------------------------------------------------
```
### Documentation (Optional)
- In case of new functionality, my PR adds documentation in the following wiki page:
(Link the GitHub wiki you added)
### Commits
- My commits all reference appropriate Apache Helix GitHub issues in their subject lines. In addition, my commits follow the guidelines from "[How to write a good git commit message](http://chris.beams.io/posts/git-commit/)":
1. Subject is separated from body by a blank line
1. Subject is limited to 50 characters (not including Jira issue reference)
1. Subject does not end with a period
1. Subject uses the imperative mood ("add", not "adding")
1. Body wraps at 72 characters
1. Body explains "what" and "why", not "how"
### Code Quality
- My diff has been formatted using helix-style.xml
(helix-style-intellij.xml if IntelliJ IDE is used)
----------------------------------------------------------------
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: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] pkuwm commented on pull request #1659: Compatible support for ZNRecord to work with Jackson 1 mapper
Posted by GitBox <gi...@apache.org>.
pkuwm commented on pull request #1659:
URL: https://github.com/apache/helix/pull/1659#issuecomment-789285585
This PR is ready to be merged, approved by @dasahcc @xyuanlu
Jackson was upgraded in helix from 1.x to 2.x (#1293). Jackson 1.x does not support Jackson 2.x annotations.
This commit rolls back Jackson 2 annotations to Jackson 1, and adds a custom CodehausJacksonAnnotationIntrospector to make Jackson 2 in helix work with Jackson 1 annotations.
----------------------------------------------------------------
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: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] pkuwm closed pull request #1659: Compatible support for ZNRecord to work with Jackson 1 mapper
Posted by GitBox <gi...@apache.org>.
pkuwm closed pull request #1659:
URL: https://github.com/apache/helix/pull/1659
----------------------------------------------------------------
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: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on pull request #1659: Compatible support for ZNRecord to work with Jackson 1 mapper
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on pull request #1659:
URL: https://github.com/apache/helix/pull/1659#issuecomment-788460069
> > Thanks for the fix. One thing, is it possible to create a util method for all the Helix code to get a modified ObjectMapper? So we don't need to do such a change again once we get rid of codehaus.
>
> @jiajunwang Good question. I thought about it too. My thought was return the ObjectMapper configured with the introspector from the util `ObjectMapper mapper = JacksonUtil.newObjectMapper();` Once we remove codehaus, we just need to remove the config in `JacksonUtil.newObjectMapper()` method.
>
> Thinking again, if we do so, there'll be mixed usages in creating an ObjectMapper: `new ObjectMapper()` and `JacksonUtil.newObjectMapper()`. After we remove codehaus, `JacksonUtil.newObjectMapper()` will just do the same as `New ObjectMapper()`, which makes `newObjectMapper()` look redundant.
>
> Plus, there are only 6 places that need to set Introspector in this PR. Once we remove the codehaus and the introspector, we just need to remove the 6 places that set introspector - pretty simple. So I just don't think we need to have the util :)
Yeah, not a big deal. I don't think we will miss any of the reference since the method will be removed in the future.
----------------------------------------------------------------
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: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] dasahcc commented on a change in pull request #1659: Compatible support for ZNRecord to work with Jackson 1 mapper
Posted by GitBox <gi...@apache.org>.
dasahcc commented on a change in pull request #1659:
URL: https://github.com/apache/helix/pull/1659#discussion_r585014425
##########
File path: zookeeper-api/src/main/java/org/apache/helix/zookeeper/introspect/CodehausJacksonIntrospector.java
##########
@@ -0,0 +1,289 @@
+package org.apache.helix.zookeeper.introspect;
+
+/*
+ * 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.
+ */
+
+import com.fasterxml.jackson.annotation.JsonInclude;
+import com.fasterxml.jackson.annotation.JsonInclude.Include;
+import com.fasterxml.jackson.databind.JavaType;
+import com.fasterxml.jackson.databind.PropertyName;
+import com.fasterxml.jackson.databind.annotation.JsonSerialize.Typing;
+import com.fasterxml.jackson.databind.introspect.Annotated;
+import com.fasterxml.jackson.databind.introspect.AnnotatedClass;
+import com.fasterxml.jackson.databind.introspect.AnnotatedField;
+import com.fasterxml.jackson.databind.introspect.AnnotatedMember;
+import com.fasterxml.jackson.databind.introspect.AnnotatedMethod;
+import com.fasterxml.jackson.databind.introspect.AnnotatedParameter;
+import com.fasterxml.jackson.databind.introspect.NopAnnotationIntrospector;
+import org.codehaus.jackson.annotate.JsonCreator;
+import org.codehaus.jackson.annotate.JsonIgnore;
+import org.codehaus.jackson.annotate.JsonIgnoreProperties;
+import org.codehaus.jackson.annotate.JsonProperty;
+import org.codehaus.jackson.map.JsonSerializer;
+import org.codehaus.jackson.map.annotate.JsonSerialize;
+import org.codehaus.jackson.map.annotate.NoClass;
+
+/**
+ * This introspector works with jackson 1 annotations used in
+ * {@link org.apache.helix.zookeeper.datamodel.ZNRecord}.
+ * This is supposed used ONLY in Helix as it could be deprecated in the next major release.
+ */
+// TODO: remove this class once upgrading to Jackson 2.x in ZNRecord
+public class CodehausJacksonIntrospector extends NopAnnotationIntrospector {
+ private static final long serialVersionUID = 1L;
+
+ @SuppressWarnings("deprecation")
+ @Override
+ public String findEnumValue(Enum<?> value) {
+ return value.name();
+ }
+
+ @SuppressWarnings("deprecation")
+ @Override
+ public Boolean findIgnoreUnknownProperties(AnnotatedClass ac) {
+ JsonIgnoreProperties ignore = ac.getAnnotation(JsonIgnoreProperties.class);
+ return (ignore == null) ? null : ignore.ignoreUnknown();
+ }
+
+ @SuppressWarnings("deprecation")
+ @Override
+ public String[] findPropertiesToIgnore(Annotated ac) {
+ JsonIgnoreProperties ignore = ac.getAnnotation(JsonIgnoreProperties.class);
+ return (ignore == null) ? null : ignore.value();
+ }
+
+ @SuppressWarnings("deprecation")
+ @Override
+ public Class<?> findDeserializationContentType(Annotated am, JavaType baseContentType) {
+ JsonSerialize ann = am.getAnnotation(JsonSerialize.class);
+ if (ann != null) {
+ Class<?> cls = ann.contentAs();
+ if (cls != NoClass.class) {
+ return cls;
+ }
+ }
+ return null;
+ }
+
+ @Override
+ public PropertyName findNameForDeserialization(Annotated a) {
+ String name = null;
+ if (a instanceof AnnotatedField) {
+ name = findDeserializationName((AnnotatedField) a);
+ } else if (a instanceof AnnotatedMethod) {
+ name = findDeserializationName((AnnotatedMethod) a);
+ } else if (a instanceof AnnotatedParameter) {
+ name = findDeserializationName((AnnotatedParameter) a);
+ }
+ if (name == null) {
+ return null;
+ }
+ return name.isEmpty() ? PropertyName.USE_DEFAULT : new PropertyName(name);
+ }
+
+ public String findDeserializationName(AnnotatedField af) {
+ JsonProperty pann = af.getAnnotation(JsonProperty.class);
+ return pann == null ? null : pann.value();
+ }
+
+ public String findDeserializationName(AnnotatedMethod am) {
+ JsonProperty pann = am.getAnnotation(JsonProperty.class);
+ return pann == null ? null : pann.value();
+ }
+
+ public String findDeserializationName(AnnotatedParameter param) {
+ if (param != null) {
+ JsonProperty pann = param.getAnnotation(JsonProperty.class);
+ if (pann != null) {
+ return pann.value();
+ }
+ }
+ return null;
+ }
+
+ @SuppressWarnings("deprecation")
Review comment:
Question: This is deprecated. Do we need to override it?
----------------------------------------------------------------
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: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] dasahcc commented on a change in pull request #1659: Compatible support for ZNRecord to work with Jackson 1 mapper
Posted by GitBox <gi...@apache.org>.
dasahcc commented on a change in pull request #1659:
URL: https://github.com/apache/helix/pull/1659#discussion_r585013918
##########
File path: zookeeper-api/src/main/java/org/apache/helix/zookeeper/introspect/CodehausJacksonIntrospector.java
##########
@@ -0,0 +1,289 @@
+package org.apache.helix.zookeeper.introspect;
+
+/*
+ * 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.
+ */
+
+import com.fasterxml.jackson.annotation.JsonInclude;
+import com.fasterxml.jackson.annotation.JsonInclude.Include;
+import com.fasterxml.jackson.databind.JavaType;
+import com.fasterxml.jackson.databind.PropertyName;
+import com.fasterxml.jackson.databind.annotation.JsonSerialize.Typing;
+import com.fasterxml.jackson.databind.introspect.Annotated;
+import com.fasterxml.jackson.databind.introspect.AnnotatedClass;
+import com.fasterxml.jackson.databind.introspect.AnnotatedField;
+import com.fasterxml.jackson.databind.introspect.AnnotatedMember;
+import com.fasterxml.jackson.databind.introspect.AnnotatedMethod;
+import com.fasterxml.jackson.databind.introspect.AnnotatedParameter;
+import com.fasterxml.jackson.databind.introspect.NopAnnotationIntrospector;
+import org.codehaus.jackson.annotate.JsonCreator;
+import org.codehaus.jackson.annotate.JsonIgnore;
+import org.codehaus.jackson.annotate.JsonIgnoreProperties;
+import org.codehaus.jackson.annotate.JsonProperty;
+import org.codehaus.jackson.map.JsonSerializer;
+import org.codehaus.jackson.map.annotate.JsonSerialize;
+import org.codehaus.jackson.map.annotate.NoClass;
+
+/**
+ * This introspector works with jackson 1 annotations used in
+ * {@link org.apache.helix.zookeeper.datamodel.ZNRecord}.
+ * This is supposed used ONLY in Helix as it could be deprecated in the next major release.
+ */
+// TODO: remove this class once upgrading to Jackson 2.x in ZNRecord
+public class CodehausJacksonIntrospector extends NopAnnotationIntrospector {
Review comment:
This is class we implemented, right? Then let's try to make the naming more readable.
----------------------------------------------------------------
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: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] xyuanlu commented on a change in pull request #1659: Compatible support for ZNRecord to work with Jackson 1 mapper
Posted by GitBox <gi...@apache.org>.
xyuanlu commented on a change in pull request #1659:
URL: https://github.com/apache/helix/pull/1659#discussion_r585022799
##########
File path: helix-core/src/test/java/org/apache/helix/manager/zk/TestZNRecordSerializer.java
##########
@@ -271,4 +278,83 @@ public void testCompression() {
runId = runId + 1;
}
}
+
+ /*
+ * Tests Jackson 1.x can work with ZNRecord
+ */
+ @Test
+ public void testCodehausJacksonSerializer() {
+ ZNRecord record = createZnRecord();
+
+ ZNRecordSerializer znRecordSerializer = new ZNRecordSerializer();
+ CodehausJsonSerializer<ZNRecord> codehausJsonSerializer =
+ new CodehausJsonSerializer<>(ZNRecord.class);
+
+ byte[] codehausBytes = codehausJsonSerializer.serialize(record);
+
+ ZNRecord deserialized =
+ (ZNRecord) codehausJsonSerializer.deserialize(codehausBytes);
+ Assert.assertEquals(deserialized, record,
+ "Codehaus jackson serializer should work with ZNRecord");
+
+ deserialized = (ZNRecord) znRecordSerializer.deserialize(codehausBytes);
+ Assert.assertEquals(deserialized, record,
+ "ZNRecordSerializer should deserialize bytes serialized by codehaus serializer");
+
+ deserialized =
+ (ZNRecord) codehausJsonSerializer.deserialize(znRecordSerializer.serialize(record));
+ Assert.assertEquals(deserialized, record,
+ "codehaus serializer should deserialize bytes serialized by ZNRecordSerializer");
+ }
+
+ private static class CodehausJsonSerializer<T> {
+ private static final Logger LOG = LoggerFactory.getLogger(CodehausJsonSerializer.class);
+
+ private final Class<T> _clazz;
+ private final ObjectMapper mapper = new ObjectMapper();
+
+ public CodehausJsonSerializer(Class<T> clazz) {
+ _clazz = clazz;
+ }
+
+ public byte[] serialize(Object data) {
+ SerializationConfig serializationConfig = mapper.getSerializationConfig();
+ serializationConfig.set(SerializationConfig.Feature.INDENT_OUTPUT, true);
+ serializationConfig.set(SerializationConfig.Feature.AUTO_DETECT_FIELDS, true);
+ serializationConfig.set(SerializationConfig.Feature.CAN_OVERRIDE_ACCESS_MODIFIERS, true);
+ StringWriter sw = new StringWriter();
+
+ try {
+ mapper.writeValue(sw, data);
+
Review comment:
nit: extra line
Change LGTM :D
----------------------------------------------------------------
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: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] pkuwm merged pull request #1659: Compatible support for ZNRecord to work with Jackson 1 mapper
Posted by GitBox <gi...@apache.org>.
pkuwm merged pull request #1659:
URL: https://github.com/apache/helix/pull/1659
----------------------------------------------------------------
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: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] xyuanlu commented on a change in pull request #1659: Compatible support for ZNRecord to work with Jackson 1 mapper
Posted by GitBox <gi...@apache.org>.
xyuanlu commented on a change in pull request #1659:
URL: https://github.com/apache/helix/pull/1659#discussion_r585022799
##########
File path: helix-core/src/test/java/org/apache/helix/manager/zk/TestZNRecordSerializer.java
##########
@@ -271,4 +278,83 @@ public void testCompression() {
runId = runId + 1;
}
}
+
+ /*
+ * Tests Jackson 1.x can work with ZNRecord
+ */
+ @Test
+ public void testCodehausJacksonSerializer() {
+ ZNRecord record = createZnRecord();
+
+ ZNRecordSerializer znRecordSerializer = new ZNRecordSerializer();
+ CodehausJsonSerializer<ZNRecord> codehausJsonSerializer =
+ new CodehausJsonSerializer<>(ZNRecord.class);
+
+ byte[] codehausBytes = codehausJsonSerializer.serialize(record);
+
+ ZNRecord deserialized =
+ (ZNRecord) codehausJsonSerializer.deserialize(codehausBytes);
+ Assert.assertEquals(deserialized, record,
+ "Codehaus jackson serializer should work with ZNRecord");
+
+ deserialized = (ZNRecord) znRecordSerializer.deserialize(codehausBytes);
+ Assert.assertEquals(deserialized, record,
+ "ZNRecordSerializer should deserialize bytes serialized by codehaus serializer");
+
+ deserialized =
+ (ZNRecord) codehausJsonSerializer.deserialize(znRecordSerializer.serialize(record));
+ Assert.assertEquals(deserialized, record,
+ "codehaus serializer should deserialize bytes serialized by ZNRecordSerializer");
+ }
+
+ private static class CodehausJsonSerializer<T> {
+ private static final Logger LOG = LoggerFactory.getLogger(CodehausJsonSerializer.class);
+
+ private final Class<T> _clazz;
+ private final ObjectMapper mapper = new ObjectMapper();
+
+ public CodehausJsonSerializer(Class<T> clazz) {
+ _clazz = clazz;
+ }
+
+ public byte[] serialize(Object data) {
+ SerializationConfig serializationConfig = mapper.getSerializationConfig();
+ serializationConfig.set(SerializationConfig.Feature.INDENT_OUTPUT, true);
+ serializationConfig.set(SerializationConfig.Feature.AUTO_DETECT_FIELDS, true);
+ serializationConfig.set(SerializationConfig.Feature.CAN_OVERRIDE_ACCESS_MODIFIERS, true);
+ StringWriter sw = new StringWriter();
+
+ try {
+ mapper.writeValue(sw, data);
+
Review comment:
nit: extra line
----------------------------------------------------------------
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: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] zhangmeng916 commented on a change in pull request #1659: Compatible support for ZNRecord to work with Jackson 1 mapper
Posted by GitBox <gi...@apache.org>.
zhangmeng916 commented on a change in pull request #1659:
URL: https://github.com/apache/helix/pull/1659#discussion_r585161844
##########
File path: metadata-store-directory-common/pom.xml
##########
@@ -47,7 +47,7 @@
<dependency>
<groupId>com.fasterxml.jackson.core</groupId>
<artifactId>jackson-databind</artifactId>
- <version>2.10.2</version>
+ <version>2.11.0</version>
Review comment:
I'm a bit confused about the lib changes in several pom files. Would you explain a bit? Also does it require any ivy file change later?
##########
File path: helix-core/src/test/java/org/apache/helix/TestShuffledIdealState.java
##########
@@ -65,7 +66,8 @@ public void testInvocation() throws Exception {
IdealCalculatorByConsistentHashing.printNodeOfflineOverhead(result3);
// System.out.println(result);
- ObjectMapper mapper = new ObjectMapper();
+ ObjectMapper mapper =
Review comment:
Is this the only test that you picked to change?
----------------------------------------------------------------
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: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] pkuwm commented on a change in pull request #1659: Compatible support for ZNRecord to work with Jackson 1 mapper
Posted by GitBox <gi...@apache.org>.
pkuwm commented on a change in pull request #1659:
URL: https://github.com/apache/helix/pull/1659#discussion_r585173299
##########
File path: metadata-store-directory-common/pom.xml
##########
@@ -47,7 +47,7 @@
<dependency>
<groupId>com.fasterxml.jackson.core</groupId>
<artifactId>jackson-databind</artifactId>
- <version>2.10.2</version>
+ <version>2.11.0</version>
Review comment:
Sure.
1. jackson-databind depends on jackson-core, so no need to explicitly specify jackson-core
2. If jackson 2.11.0 is used in helix, I believe it is better to make the version consistent. So change 2.10.2 -> 2.11.0
3. Yeah, better to also change the version in Ivy.
##########
File path: helix-core/src/test/java/org/apache/helix/TestShuffledIdealState.java
##########
@@ -65,7 +66,8 @@ public void testInvocation() throws Exception {
IdealCalculatorByConsistentHashing.printNodeOfflineOverhead(result3);
// System.out.println(result);
- ObjectMapper mapper = new ObjectMapper();
+ ObjectMapper mapper =
Review comment:
If an ObjectMapper needs to deserialize ZNRecord, it needs to set the introspector.
This test is the only test in helix-core that explicitly creates an ObjectMapper to work with ZNRecord. If we want to do better, this test should use the `ZNRecordSerializer` to deserialize it. To make the code minimum, I just don't want to do the refactoring work.
----------------------------------------------------------------
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: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] pkuwm commented on a change in pull request #1659: Compatible support for ZNRecord to work with Jackson 1 mapper
Posted by GitBox <gi...@apache.org>.
pkuwm commented on a change in pull request #1659:
URL: https://github.com/apache/helix/pull/1659#discussion_r585071056
##########
File path: zookeeper-api/src/main/java/org/apache/helix/zookeeper/introspect/CodehausJacksonIntrospector.java
##########
@@ -0,0 +1,289 @@
+package org.apache.helix.zookeeper.introspect;
+
+/*
+ * 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.
+ */
+
+import com.fasterxml.jackson.annotation.JsonInclude;
+import com.fasterxml.jackson.annotation.JsonInclude.Include;
+import com.fasterxml.jackson.databind.JavaType;
+import com.fasterxml.jackson.databind.PropertyName;
+import com.fasterxml.jackson.databind.annotation.JsonSerialize.Typing;
+import com.fasterxml.jackson.databind.introspect.Annotated;
+import com.fasterxml.jackson.databind.introspect.AnnotatedClass;
+import com.fasterxml.jackson.databind.introspect.AnnotatedField;
+import com.fasterxml.jackson.databind.introspect.AnnotatedMember;
+import com.fasterxml.jackson.databind.introspect.AnnotatedMethod;
+import com.fasterxml.jackson.databind.introspect.AnnotatedParameter;
+import com.fasterxml.jackson.databind.introspect.NopAnnotationIntrospector;
+import org.codehaus.jackson.annotate.JsonCreator;
+import org.codehaus.jackson.annotate.JsonIgnore;
+import org.codehaus.jackson.annotate.JsonIgnoreProperties;
+import org.codehaus.jackson.annotate.JsonProperty;
+import org.codehaus.jackson.map.JsonSerializer;
+import org.codehaus.jackson.map.annotate.JsonSerialize;
+import org.codehaus.jackson.map.annotate.NoClass;
+
+/**
+ * This introspector works with jackson 1 annotations used in
+ * {@link org.apache.helix.zookeeper.datamodel.ZNRecord}.
+ * This is supposed used ONLY in Helix as it could be deprecated in the next major release.
+ */
+// TODO: remove this class once upgrading to Jackson 2.x in ZNRecord
+public class CodehausJacksonIntrospector extends NopAnnotationIntrospector {
Review comment:
Addressed. Thanks.
##########
File path: zookeeper-api/src/main/java/org/apache/helix/zookeeper/introspect/CodehausJacksonIntrospector.java
##########
@@ -0,0 +1,289 @@
+package org.apache.helix.zookeeper.introspect;
+
+/*
+ * 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.
+ */
+
+import com.fasterxml.jackson.annotation.JsonInclude;
+import com.fasterxml.jackson.annotation.JsonInclude.Include;
+import com.fasterxml.jackson.databind.JavaType;
+import com.fasterxml.jackson.databind.PropertyName;
+import com.fasterxml.jackson.databind.annotation.JsonSerialize.Typing;
+import com.fasterxml.jackson.databind.introspect.Annotated;
+import com.fasterxml.jackson.databind.introspect.AnnotatedClass;
+import com.fasterxml.jackson.databind.introspect.AnnotatedField;
+import com.fasterxml.jackson.databind.introspect.AnnotatedMember;
+import com.fasterxml.jackson.databind.introspect.AnnotatedMethod;
+import com.fasterxml.jackson.databind.introspect.AnnotatedParameter;
+import com.fasterxml.jackson.databind.introspect.NopAnnotationIntrospector;
+import org.codehaus.jackson.annotate.JsonCreator;
+import org.codehaus.jackson.annotate.JsonIgnore;
+import org.codehaus.jackson.annotate.JsonIgnoreProperties;
+import org.codehaus.jackson.annotate.JsonProperty;
+import org.codehaus.jackson.map.JsonSerializer;
+import org.codehaus.jackson.map.annotate.JsonSerialize;
+import org.codehaus.jackson.map.annotate.NoClass;
+
+/**
+ * This introspector works with jackson 1 annotations used in
+ * {@link org.apache.helix.zookeeper.datamodel.ZNRecord}.
+ * This is supposed used ONLY in Helix as it could be deprecated in the next major release.
+ */
+// TODO: remove this class once upgrading to Jackson 2.x in ZNRecord
+public class CodehausJacksonIntrospector extends NopAnnotationIntrospector {
+ private static final long serialVersionUID = 1L;
+
+ @SuppressWarnings("deprecation")
+ @Override
+ public String findEnumValue(Enum<?> value) {
+ return value.name();
+ }
+
+ @SuppressWarnings("deprecation")
+ @Override
+ public Boolean findIgnoreUnknownProperties(AnnotatedClass ac) {
+ JsonIgnoreProperties ignore = ac.getAnnotation(JsonIgnoreProperties.class);
+ return (ignore == null) ? null : ignore.ignoreUnknown();
+ }
+
+ @SuppressWarnings("deprecation")
+ @Override
+ public String[] findPropertiesToIgnore(Annotated ac) {
+ JsonIgnoreProperties ignore = ac.getAnnotation(JsonIgnoreProperties.class);
+ return (ignore == null) ? null : ignore.value();
+ }
+
+ @SuppressWarnings("deprecation")
+ @Override
+ public Class<?> findDeserializationContentType(Annotated am, JavaType baseContentType) {
+ JsonSerialize ann = am.getAnnotation(JsonSerialize.class);
+ if (ann != null) {
+ Class<?> cls = ann.contentAs();
+ if (cls != NoClass.class) {
+ return cls;
+ }
+ }
+ return null;
+ }
+
+ @Override
+ public PropertyName findNameForDeserialization(Annotated a) {
+ String name = null;
+ if (a instanceof AnnotatedField) {
+ name = findDeserializationName((AnnotatedField) a);
+ } else if (a instanceof AnnotatedMethod) {
+ name = findDeserializationName((AnnotatedMethod) a);
+ } else if (a instanceof AnnotatedParameter) {
+ name = findDeserializationName((AnnotatedParameter) a);
+ }
+ if (name == null) {
+ return null;
+ }
+ return name.isEmpty() ? PropertyName.USE_DEFAULT : new PropertyName(name);
+ }
+
+ public String findDeserializationName(AnnotatedField af) {
+ JsonProperty pann = af.getAnnotation(JsonProperty.class);
+ return pann == null ? null : pann.value();
+ }
+
+ public String findDeserializationName(AnnotatedMethod am) {
+ JsonProperty pann = am.getAnnotation(JsonProperty.class);
+ return pann == null ? null : pann.value();
+ }
+
+ public String findDeserializationName(AnnotatedParameter param) {
+ if (param != null) {
+ JsonProperty pann = param.getAnnotation(JsonProperty.class);
+ if (pann != null) {
+ return pann.value();
+ }
+ }
+ return null;
+ }
+
+ @SuppressWarnings("deprecation")
Review comment:
As I checked the Jackson 2.11 source code, some methods marked as deprecated are still called in the non-deprecated methods, eg. `hasCreatorAnnotation()` ([link](https://github.com/FasterXML/jackson-databind/blob/eb3ebd90b6303610ff60a7fba33b04691957930b/src/main/java/com/fasterxml/jackson/databind/AnnotationIntrospector.java#L1304)) If we don't override it, the logic won't work as expected.
To make sure the code is working correctly, I prefer to keep it. It won't hurt. And we don't need to maintain this class - we'll remove it once we completely upgrade to Jackson 2 annotations.
I've also removed a few deprecated methods that we don't need, by debugging and making sure they are not called.
##########
File path: helix-core/src/test/java/org/apache/helix/manager/zk/TestZNRecordSerializer.java
##########
@@ -271,4 +278,83 @@ public void testCompression() {
runId = runId + 1;
}
}
+
+ /*
+ * Tests Jackson 1.x can work with ZNRecord
+ */
+ @Test
+ public void testCodehausJacksonSerializer() {
+ ZNRecord record = createZnRecord();
+
+ ZNRecordSerializer znRecordSerializer = new ZNRecordSerializer();
+ CodehausJsonSerializer<ZNRecord> codehausJsonSerializer =
+ new CodehausJsonSerializer<>(ZNRecord.class);
+
+ byte[] codehausBytes = codehausJsonSerializer.serialize(record);
+
+ ZNRecord deserialized =
+ (ZNRecord) codehausJsonSerializer.deserialize(codehausBytes);
+ Assert.assertEquals(deserialized, record,
+ "Codehaus jackson serializer should work with ZNRecord");
+
+ deserialized = (ZNRecord) znRecordSerializer.deserialize(codehausBytes);
+ Assert.assertEquals(deserialized, record,
+ "ZNRecordSerializer should deserialize bytes serialized by codehaus serializer");
+
+ deserialized =
+ (ZNRecord) codehausJsonSerializer.deserialize(znRecordSerializer.serialize(record));
+ Assert.assertEquals(deserialized, record,
+ "codehaus serializer should deserialize bytes serialized by ZNRecordSerializer");
+ }
+
+ private static class CodehausJsonSerializer<T> {
+ private static final Logger LOG = LoggerFactory.getLogger(CodehausJsonSerializer.class);
+
+ private final Class<T> _clazz;
+ private final ObjectMapper mapper = new ObjectMapper();
+
+ public CodehausJsonSerializer(Class<T> clazz) {
+ _clazz = clazz;
+ }
+
+ public byte[] serialize(Object data) {
+ SerializationConfig serializationConfig = mapper.getSerializationConfig();
+ serializationConfig.set(SerializationConfig.Feature.INDENT_OUTPUT, true);
+ serializationConfig.set(SerializationConfig.Feature.AUTO_DETECT_FIELDS, true);
+ serializationConfig.set(SerializationConfig.Feature.CAN_OVERRIDE_ACCESS_MODIFIERS, true);
+ StringWriter sw = new StringWriter();
+
+ try {
+ mapper.writeValue(sw, data);
+
Review comment:
I usually put an empty line before `return` :)
But for the case here, it does not help with readability, as there is only one line of code before return. Will delete the new line.
----------------------------------------------------------------
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: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] dasahcc commented on a change in pull request #1659: Compatible support for ZNRecord to work with Jackson 1 mapper
Posted by GitBox <gi...@apache.org>.
dasahcc commented on a change in pull request #1659:
URL: https://github.com/apache/helix/pull/1659#discussion_r585013918
##########
File path: zookeeper-api/src/main/java/org/apache/helix/zookeeper/introspect/CodehausJacksonIntrospector.java
##########
@@ -0,0 +1,289 @@
+package org.apache.helix.zookeeper.introspect;
+
+/*
+ * 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.
+ */
+
+import com.fasterxml.jackson.annotation.JsonInclude;
+import com.fasterxml.jackson.annotation.JsonInclude.Include;
+import com.fasterxml.jackson.databind.JavaType;
+import com.fasterxml.jackson.databind.PropertyName;
+import com.fasterxml.jackson.databind.annotation.JsonSerialize.Typing;
+import com.fasterxml.jackson.databind.introspect.Annotated;
+import com.fasterxml.jackson.databind.introspect.AnnotatedClass;
+import com.fasterxml.jackson.databind.introspect.AnnotatedField;
+import com.fasterxml.jackson.databind.introspect.AnnotatedMember;
+import com.fasterxml.jackson.databind.introspect.AnnotatedMethod;
+import com.fasterxml.jackson.databind.introspect.AnnotatedParameter;
+import com.fasterxml.jackson.databind.introspect.NopAnnotationIntrospector;
+import org.codehaus.jackson.annotate.JsonCreator;
+import org.codehaus.jackson.annotate.JsonIgnore;
+import org.codehaus.jackson.annotate.JsonIgnoreProperties;
+import org.codehaus.jackson.annotate.JsonProperty;
+import org.codehaus.jackson.map.JsonSerializer;
+import org.codehaus.jackson.map.annotate.JsonSerialize;
+import org.codehaus.jackson.map.annotate.NoClass;
+
+/**
+ * This introspector works with jackson 1 annotations used in
+ * {@link org.apache.helix.zookeeper.datamodel.ZNRecord}.
+ * This is supposed used ONLY in Helix as it could be deprecated in the next major release.
+ */
+// TODO: remove this class once upgrading to Jackson 2.x in ZNRecord
+public class CodehausJacksonIntrospector extends NopAnnotationIntrospector {
Review comment:
This is class we implemented, right? Then let's try to make the variables naming more readable.
----------------------------------------------------------------
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: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang edited a comment on pull request #1659: Compatible support for ZNRecord to work with Jackson 1 mapper
Posted by GitBox <gi...@apache.org>.
jiajunwang edited a comment on pull request #1659:
URL: https://github.com/apache/helix/pull/1659#issuecomment-788460069
> > Thanks for the fix. One thing, is it possible to create a util method for all the Helix code to get a modified ObjectMapper? So we don't need to do such a change again once we get rid of codehaus.
>
> @jiajunwang Good question. I thought about it too. My thought was return the ObjectMapper configured with the introspector from the util `ObjectMapper mapper = JacksonUtil.newObjectMapper();` Once we remove codehaus, we just need to remove the config in `JacksonUtil.newObjectMapper()` method.
>
> Thinking again, if we do so, there'll be mixed usages in creating an ObjectMapper: `new ObjectMapper()` and `JacksonUtil.newObjectMapper()`. After we remove codehaus, `JacksonUtil.newObjectMapper()` will just do the same as `New ObjectMapper()`, which makes `newObjectMapper()` look redundant.
>
> Plus, there are only 6 places that need to set Introspector in this PR. Once we remove the codehaus and the introspector, we just need to remove the 6 places that set introspector - pretty simple. So I just don't think we need to have the util :)
Yeah, not a big deal. I don't think we will miss any of the references since the method will be removed in the future.
----------------------------------------------------------------
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: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org