You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zookeeper.apache.org by alburthoffman <gi...@git.apache.org> on 2017/04/24 06:08:59 UTC
[GitHub] zookeeper pull request #238: ZOOKEEPER-2763: Utils.toCsvBuffer() omits leadi...
GitHub user alburthoffman opened a pull request:
https://github.com/apache/zookeeper/pull/238
ZOOKEEPER-2763: Utils.toCsvBuffer() omits leading 0 for bytes < 0x10
1. fix toCsvBuffer() and toXMLBuffer()
2. reduce duplicate code
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/alburthoffman/zookeeper ZOOKEEPER_2763
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/zookeeper/pull/238.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #238
----
commit a1461e47e89878f8772b2136f1f2ff978b1f6f4b
Author: wtang3 <wt...@ebay.com>
Date: 2017-04-24T05:46:57Z
fix https://issues.apache.org/jira/browse/ZOOKEEPER-2763
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #238: ZOOKEEPER-2763: Utils.toCsvBuffer() omits leadi...
Posted by alburthoffman <gi...@git.apache.org>.
Github user alburthoffman commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/238#discussion_r120258026
--- Diff: src/java/main/org/apache/jute/Utils.java ---
@@ -190,19 +190,32 @@ static String fromCSVString(String s) throws IOException {
}
/**
+ * convert byte array to a string in hex format
*
- * @param s
- * @return
+ * @param barr
+ * @return
*/
- static String toXMLBuffer(byte barr[]) {
--- End diff --
@afine I belive it's the testing environment, because the tests for datatypeconverter will fail. here is what I found from their jenkins job console:
java version "1.7.0_80"
Java(TM) SE Runtime Environment (build 1.7.0_80-b15)
Java HotSpot(TM) 64-Bit Server VM (build 24.80-b11, mixed mode)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #238: ZOOKEEPER-2763: Utils.toCsvBuffer() omits leadi...
Posted by alburthoffman <gi...@git.apache.org>.
Github user alburthoffman commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/238#discussion_r113356883
--- Diff: src/java/main/org/apache/jute/Utils.java ---
@@ -190,19 +190,32 @@ static String fromCSVString(String s) throws IOException {
}
/**
+ * convert byte array to a string in hex format
*
- * @param s
- * @return
+ * @param barr
+ * @return
*/
- static String toXMLBuffer(byte barr[]) {
--- End diff --
you are right. printHexBinary works exactly as same as toHexString.
I will change it to use printHexBinary and add test cases.
Thx for ur suggestion
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #238: ZOOKEEPER-2763: Utils.toCsvBuffer() omits leadi...
Posted by alburthoffman <gi...@git.apache.org>.
Github user alburthoffman commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/238#discussion_r117419956
--- Diff: src/java/main/org/apache/jute/Utils.java ---
@@ -190,19 +190,32 @@ static String fromCSVString(String s) throws IOException {
}
/**
+ * convert byte array to a string in hex format
*
- * @param s
- * @return
+ * @param barr
+ * @return
*/
- static String toXMLBuffer(byte barr[]) {
--- End diff --
I'm not able to dig into Zookeeper internal to find the root cause.
but this link would help:
http://stackoverflow.com/questions/12498256/why-the-npe-using-static-method-of-datatypeconverter
https://developer.ibm.com/answers/questions/200553/jaxb-datatypeconverter-throws-nullpointerexception.html
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #238: ZOOKEEPER-2763: Utils.toCsvBuffer() omits leading 0 fo...
Posted by alburthoffman <gi...@git.apache.org>.
Github user alburthoffman commented on the issue:
https://github.com/apache/zookeeper/pull/238
@eribeiro you are correct. foreach loop is more readable.
But for bufEquals, it should be in another commit(code refractor?). cause this PR is for ZOOKEEPER-2763
let's make it simple.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #238: ZOOKEEPER-2763: Utils.toCsvBuffer() omits leading 0 fo...
Posted by chelin <gi...@git.apache.org>.
Github user chelin commented on the issue:
https://github.com/apache/zookeeper/pull/238
Regarding
> for (char ch : s.toCharArray())
The difference is that toCharArray() results in a copy of the intrinsic array, leading to increased memory usage
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #238: ZOOKEEPER-2763: Utils.toCsvBuffer() omits leadi...
Posted by afine <gi...@git.apache.org>.
Github user afine commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/238#discussion_r113297764
--- Diff: src/java/main/org/apache/jute/Utils.java ---
@@ -190,19 +190,32 @@ static String fromCSVString(String s) throws IOException {
}
/**
+ * convert byte array to a string in hex format
*
- * @param s
- * @return
+ * @param barr
+ * @return
*/
- static String toXMLBuffer(byte barr[]) {
--- End diff --
Great catch @alburthoffman
Just wondering if there is anything wrong with java's built in XML tooling here. Using the example from the JIRA:
`javax.xml.bind.DatatypeConverter.printHexBinary(new byte[] {0x10, 0x05, -0x20})` => "1005E0"
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #238: ZOOKEEPER-2763: Utils.toCsvBuffer() omits leading 0 fo...
Posted by eribeiro <gi...@git.apache.org>.
Github user eribeiro commented on the issue:
https://github.com/apache/zookeeper/pull/238
@alburthoffman @afine Hey guys, did you notice that the body of `bufEquals` method could be replaced by `return Arrays.equals(onearray, twoarray);`?
In fact, the current implementation of `bufEquals` is almost a clone of `Arrays.equals`
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #238: ZOOKEEPER-2763: Utils.toCsvBuffer() omits leadi...
Posted by afine <gi...@git.apache.org>.
Github user afine commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/238#discussion_r117538863
--- Diff: src/java/main/org/apache/jute/Utils.java ---
@@ -190,19 +190,32 @@ static String fromCSVString(String s) throws IOException {
}
/**
+ * convert byte array to a string in hex format
*
- * @param s
- * @return
+ * @param barr
+ * @return
*/
- static String toXMLBuffer(byte barr[]) {
--- End diff --
I went through the links that you provided. Would you mind including the version of the JVM that you are running? According to https://issues.apache.org/jira/browse/CAMEL-4893 the issue I think you are running into was fixed in JDK's greater than 1.7.0_02.
It would also be great if you could include a stack trace of the issue you are running into.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #238: ZOOKEEPER-2763: Utils.toCsvBuffer() omits leadi...
Posted by afine <gi...@git.apache.org>.
Github user afine commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/238#discussion_r120479760
--- Diff: src/java/main/org/apache/jute/Utils.java ---
@@ -190,19 +190,32 @@ static String fromCSVString(String s) throws IOException {
}
/**
+ * convert byte array to a string in hex format
*
- * @param s
- * @return
+ * @param barr
+ * @return
*/
- static String toXMLBuffer(byte barr[]) {
--- End diff --
Hi @alburthoffman -
I have been trying to reproduce the issue that you described. A stack trace would be helpful here. Do you know exactly which test was failing for you (as I don't think the logs for any relevant jenkins job still exists)? I was able to get the whole test suite to pass with `DatatypeConverter.printHexBinary`.
Although, if I do not add null check I can get `testJuteToString` to fail.
```
java.lang.NullPointerException
at javax.xml.bind.DatatypeConverterImpl.printHexBinary(DatatypeConverterImpl.java:479)
at javax.xml.bind.DatatypeConverter.printHexBinary(DatatypeConverter.java:626)
at org.apache.jute.Utils.toHexString(Utils.java:203)
at org.apache.jute.Utils.toCSVBuffer(Utils.java:242)
at org.apache.jute.CsvOutputArchive.writeBuffer(CsvOutputArchive.java:100)
at org.apache.zookeeper.proto.SetDataRequest.toString(SetDataRequest.java:77)
at org.apache.zookeeper.server.ToStringTest.testJuteToString(ToStringTest.java:36)
```
I was able to remedy this with a null check before the javax call.
```
private static String toHexString(byte barr[]) {
if (barr == null) {
return "";
}
return javax.xml.bind.DatatypeConverter.printHexBinary(barr);
}
```
Thanks for your patience on this.
Abe
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #238: ZOOKEEPER-2763: Utils.toCsvBuffer() omits leadi...
Posted by alburthoffman <gi...@git.apache.org>.
Github user alburthoffman commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/238#discussion_r133361758
--- Diff: src/java/main/org/apache/jute/Utils.java ---
@@ -190,19 +190,32 @@ static String fromCSVString(String s) throws IOException {
}
/**
+ * convert byte array to a string in hex format
*
- * @param s
- * @return
+ * @param barr
+ * @return
*/
- static String toXMLBuffer(byte barr[]) {
--- End diff --
@afine I changed the code to use DatatypeConverter.printHexBinary(barr). but the tests keep failing.
I take a look at the failures. it's very wired, because should be caused by this change.
Also I got different test cases failure in local and in hadoop jenkins report.
Not sure how to trigger hadoop jenkins job to run tests, so no way to paste it here.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #238: ZOOKEEPER-2763: Utils.toCsvBuffer() omits leading 0 fo...
Posted by eribeiro <gi...@git.apache.org>.
Github user eribeiro commented on the issue:
https://github.com/apache/zookeeper/pull/238
Also, loops like the one below:
```
for (int idx = 0; idx < s.length(); idx++) {
char ch = s.charAt(idx);
```
could be simplified to `for (char ch : s.toCharArray()) {` in some **very specific methods that don't increment the counter inside the loop**.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #238: ZOOKEEPER-2763: Utils.toCsvBuffer() omits leadi...
Posted by alburthoffman <gi...@git.apache.org>.
Github user alburthoffman commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/238#discussion_r113880584
--- Diff: src/java/main/org/apache/jute/Utils.java ---
@@ -190,19 +190,32 @@ static String fromCSVString(String s) throws IOException {
}
/**
+ * convert byte array to a string in hex format
*
- * @param s
- * @return
+ * @param barr
+ * @return
*/
- static String toXMLBuffer(byte barr[]) {
--- End diff --
@afine found the reason why not use DatatypeConverter:
JAXB Providers are required to call the setDatatypeConverter api at some point before the first marshal or unmarshal operation (perhaps during the call to JAXBContext.newInstance). This step is necessary to configure the converter that should be used to perform the print and parse functionality.
In utils class, initialize JAXB is too heavy, so let's make it simple....
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #238: ZOOKEEPER-2763: Utils.toCsvBuffer() omits leading 0 fo...
Posted by eribeiro <gi...@git.apache.org>.
Github user eribeiro commented on the issue:
https://github.com/apache/zookeeper/pull/238
Totally agree. It is worth another jira, for sure. :)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #238: ZOOKEEPER-2763: Utils.toCsvBuffer() omits leadi...
Posted by afine <gi...@git.apache.org>.
Github user afine commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/238#discussion_r114011352
--- Diff: src/java/main/org/apache/jute/Utils.java ---
@@ -190,19 +190,32 @@ static String fromCSVString(String s) throws IOException {
}
/**
+ * convert byte array to a string in hex format
*
- * @param s
- * @return
+ * @param barr
+ * @return
*/
- static String toXMLBuffer(byte barr[]) {
--- End diff --
Are you sure about that? I know next to nothing about JAXB so I can definitely be wrong, but I was able to drop `DatatypeConverter.printHexBinary` in place and have everything just work. Digging through the implementation and DatatypeConverter instantiation happening behind the scenes everything looked reasonable. Can you link me to something that shows what I am missing?
Thanks
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---