You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@zookeeper.apache.org by GitBox <gi...@apache.org> on 2020/07/23 07:10:36 UTC
[GitHub] [zookeeper] eolivelli opened a new pull request #1407: ZOOKEEPER-3895 Client side NullPointerException in case of empty Multi operation
eolivelli opened a new pull request #1407:
URL: https://github.com/apache/zookeeper/pull/1407
https://issues.apache.org/jira/browse/ZOOKEEPER-3895
----------------------------------------------------------------
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
[GitHub] [zookeeper] TisonKun commented on a change in pull request #1407: ZOOKEEPER-3895 Client side NullPointerException in case of empty Multi operation
Posted by GitBox <gi...@apache.org>.
TisonKun commented on a change in pull request #1407:
URL: https://github.com/apache/zookeeper/pull/1407#discussion_r459373435
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/ClientCnxn.java
##########
@@ -589,6 +600,8 @@ private void processEvent(Object event) {
((AsyncCallback.EphemeralsCallback) lcb.cb).processResult(lcb.rc, lcb.ctx, null);
} else if (lcb.cb instanceof AsyncCallback.AllChildrenNumberCallback) {
((AsyncCallback.AllChildrenNumberCallback) lcb.cb).processResult(lcb.rc, lcb.path, lcb.ctx, -1);
+ } else if (lcb.cb instanceof AsyncCallback.MultiCallback) {
Review comment:
It seems we don't call `queueCallback` anywhere so a `LocalCallback` is never created. If so, we should delete such dead code instead of adapting 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
[GitHub] [zookeeper] symat commented on a change in pull request #1407: ZOOKEEPER-3895 Client side NullPointerException in case of empty Multi operation
Posted by GitBox <gi...@apache.org>.
symat commented on a change in pull request #1407:
URL: https://github.com/apache/zookeeper/pull/1407#discussion_r459373303
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/ClientCnxn.java
##########
@@ -469,6 +470,16 @@ private static String makeThreadName(String suffix) {
return name + suffix;
}
+ /**
+ * Tests that current thread is the main event loop.
+ * This method is useful only for tests inside ZooKeeper project
+ * it is not a public API intended for use by external applications.
+ * @return true if Thread.currentThread() is an EventThread.
+ */
+ public static boolean isInEventThread() {
Review comment:
just an idea: what about making it package private, and moving the testcase (or the whole test file) to the org.apache.zookeeper package?
----------------------------------------------------------------
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
[GitHub] [zookeeper] symat commented on pull request #1407: ZOOKEEPER-3895 Client side NullPointerException in case of empty Multi operation
Posted by GitBox <gi...@apache.org>.
symat commented on pull request #1407:
URL: https://github.com/apache/zookeeper/pull/1407#issuecomment-662945060
retest maven build
----------------------------------------------------------------
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
[GitHub] [zookeeper] asfgit closed pull request #1407: ZOOKEEPER-3895 Client side NullPointerException in case of empty Multi operation
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #1407:
URL: https://github.com/apache/zookeeper/pull/1407
----------------------------------------------------------------
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
[GitHub] [zookeeper] eolivelli edited a comment on pull request #1407: ZOOKEEPER-3895 Client side NullPointerException in case of empty Multi operation
Posted by GitBox <gi...@apache.org>.
eolivelli edited a comment on pull request #1407:
URL: https://github.com/apache/zookeeper/pull/1407#issuecomment-663558359
retest maven build
----------------------------------------------------------------
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
[GitHub] [zookeeper] symat commented on a change in pull request #1407: ZOOKEEPER-3895 Client side NullPointerException in case of empty Multi operation
Posted by GitBox <gi...@apache.org>.
symat commented on a change in pull request #1407:
URL: https://github.com/apache/zookeeper/pull/1407#discussion_r459452127
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/ClientCnxn.java
##########
@@ -469,6 +470,16 @@ private static String makeThreadName(String suffix) {
return name + suffix;
}
+ /**
+ * Tests that current thread is the main event loop.
+ * This method is useful only for tests inside ZooKeeper project
+ * it is not a public API intended for use by external applications.
+ * @return true if Thread.currentThread() is an EventThread.
+ */
+ public static boolean isInEventThread() {
Review comment:
Usually I try to avoid adding anything to the code "just for the tests". On the other hand this method can be even handy when the users are writing their own callbacks. Or for other unit tests. And the EventThread is an inner-class, so I wouldn't expose it either...
So all-in-all I think this is a meaningful change, let's keep 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
[GitHub] [zookeeper] eolivelli commented on pull request #1407: ZOOKEEPER-3895 Client side NullPointerException in case of empty Multi operation
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #1407:
URL: https://github.com/apache/zookeeper/pull/1407#issuecomment-663558359
rerun maven build
----------------------------------------------------------------
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
[GitHub] [zookeeper] symat commented on a change in pull request #1407: ZOOKEEPER-3895 Client side NullPointerException in case of empty Multi operation
Posted by GitBox <gi...@apache.org>.
symat commented on a change in pull request #1407:
URL: https://github.com/apache/zookeeper/pull/1407#discussion_r459380552
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/ClientCnxn.java
##########
@@ -589,6 +600,8 @@ private void processEvent(Object event) {
((AsyncCallback.EphemeralsCallback) lcb.cb).processResult(lcb.rc, lcb.ctx, null);
} else if (lcb.cb instanceof AsyncCallback.AllChildrenNumberCallback) {
((AsyncCallback.AllChildrenNumberCallback) lcb.cb).processResult(lcb.rc, lcb.path, lcb.ctx, -1);
+ } else if (lcb.cb instanceof AsyncCallback.MultiCallback) {
Review comment:
at the first glance, it makes sense for me... AFAICT: if the client would like to execute an "empty" multiop request, we just return immediately without even sending the request to the server.
@eolivelli Do you think we would need to implement the similar logic for the C-client? (I see we have `TestMulti.cc` with a few unit tests, but don't see any test for empty multi-request. Also I don't see in the c code that this specific case would be handled in the C client.) I think adding a new unit test for the C-client (and potentially make a fix) would make sense. Or at least register a follow-up jira for 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
[GitHub] [zookeeper] eolivelli commented on pull request #1407: ZOOKEEPER-3895 Client side NullPointerException in case of empty Multi operation
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #1407:
URL: https://github.com/apache/zookeeper/pull/1407#issuecomment-662978887
@symat
This is a bug on the Java client, probably a regression of 3.6.0.
I will create a ticket for the C-client, as I don't have a working env for C-client development during these days
@TisonKun
as you mentioned the problem is that opKind is null.
If the client requests zero ops that it is expected to have zero results, so it is not a problem from my point of view.
I think there is no need to log a warning, we should simply do nothing and return.
----------------------------------------------------------------
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
[GitHub] [zookeeper] symat commented on pull request #1407: ZOOKEEPER-3895 Client side NullPointerException in case of empty Multi operation
Posted by GitBox <gi...@apache.org>.
symat commented on pull request #1407:
URL: https://github.com/apache/zookeeper/pull/1407#issuecomment-665772754
----------------------------------------------------------------
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
[GitHub] [zookeeper] eolivelli commented on pull request #1407: ZOOKEEPER-3895 Client side NullPointerException in case of empty Multi operation
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #1407:
URL: https://github.com/apache/zookeeper/pull/1407#issuecomment-663559071
@symat Jenkins is not in good shape
see
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build-maven/2153/console
----------------------------------------------------------------
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
[GitHub] [zookeeper] eolivelli commented on a change in pull request #1407: ZOOKEEPER-3895 Client side NullPointerException in case of empty Multi operation
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #1407:
URL: https://github.com/apache/zookeeper/pull/1407#discussion_r459413237
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/ClientCnxn.java
##########
@@ -469,6 +470,16 @@ private static String makeThreadName(String suffix) {
return name + suffix;
}
+ /**
+ * Tests that current thread is the main event loop.
+ * This method is useful only for tests inside ZooKeeper project
+ * it is not a public API intended for use by external applications.
+ * @return true if Thread.currentThread() is an EventThread.
+ */
+ public static boolean isInEventThread() {
Review comment:
@symat
I made it public because I thought that this kind of assertions would be very useful while working on the EventThread.
I also didn't want to move the test case, in order to limit the impact of the patch over the codebase.
I will do it if you feel strong
----------------------------------------------------------------
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
[GitHub] [zookeeper] eolivelli commented on pull request #1407: ZOOKEEPER-3895 Client side NullPointerException in case of empty Multi operation
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #1407:
URL: https://github.com/apache/zookeeper/pull/1407#issuecomment-665560093
----------------------------------------------------------------
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
[GitHub] [zookeeper] TisonKun commented on a change in pull request #1407: ZOOKEEPER-3895 Client side NullPointerException in case of empty Multi operation
Posted by GitBox <gi...@apache.org>.
TisonKun commented on a change in pull request #1407:
URL: https://github.com/apache/zookeeper/pull/1407#discussion_r459374810
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/ClientCnxn.java
##########
@@ -589,6 +600,8 @@ private void processEvent(Object event) {
((AsyncCallback.EphemeralsCallback) lcb.cb).processResult(lcb.rc, lcb.ctx, null);
} else if (lcb.cb instanceof AsyncCallback.AllChildrenNumberCallback) {
((AsyncCallback.AllChildrenNumberCallback) lcb.cb).processResult(lcb.rc, lcb.path, lcb.ctx, -1);
+ } else if (lcb.cb instanceof AsyncCallback.MultiCallback) {
Review comment:
Hmmm I just see the next change, so we active an unused method. Let me take a closer look.
----------------------------------------------------------------
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
[GitHub] [zookeeper] symat commented on pull request #1407: ZOOKEEPER-3895 Client side NullPointerException in case of empty Multi operation
Posted by GitBox <gi...@apache.org>.
symat commented on pull request #1407:
URL: https://github.com/apache/zookeeper/pull/1407#issuecomment-664915266
> @symat Jenkins is not in good shape
yep, we have some C-client build issues. I guess something changed in the build environment:
> [INFO] --- exec-maven-plugin:1.6.0:exec (autoreconf) @ zookeeper-client-c ---
autoreconf: cannot create /tmp/user/910/ar1863.9600: No such file or directory
at /usr/bin/autoreconf line 691.
[ERROR] Command execution failed.
I don't really know our CI jobs and the apache infrastructure... I don't know if it is even worth to investigate, as we are migrating to some other jenkins right now.
Anyway, I tested your patch locally and all tests (incl. C-client) passed, so I'm good. I can merge it. Only to master (given the point raised by @TisonKun about behaviour change)? Or you planned to also merge it to branch-3.6?
----------------------------------------------------------------
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