Michael Kublin has posted comments on this change. Change subject: engine-core: Adding non-blocking VDSM api ......................................................................
Patch Set 2: (9 inline comments) .................................................... File backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/FutureVdsmTask.java Line 10: These is wrapper , which is delegation its functionality to the HttpTask Line 36: @Override What these method suppose to do? the call is gone. The result is Future. processReturnValue() will call to ProceedProxyReturnValue. The result is null, I don't shure that we will fall on NullPointerException but possible. The thread is left stacked forever. I am talking about the only one thread, that which is performing xml rpc call Line 41: Let see on stack of that method call: FutureVdsmTask.get -> httpTask.get -> xmlRpcTask.get. For me it is too long , for simple get with out any logic .................................................... File backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/HttpTask.java Line 11: These is wrapper , which is delegation all functionality to Future implementation of java Line 36: @Override These is blocking call Line 48: @Override These method usually will return a null .................................................... File backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/PollVdsCommand.java Line 16: The rule is simple: One thread pool, one thread, one future. I don't see reason for two .................................................... File backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/VdsServerConnector.java Line 29: @FutureCall(delegeteTo = "getVdsCapabilities") These is a bad name. In order to understand what is doing, I will need go to API and check annotation .................................................... File backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/VdsServerWrapper.java Line 917: And here u actually missed @Override -- To view, visit http://gerrit.ovirt.org/1727 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6d84e13fd075397a7152afee866b9767c72ca720 Gerrit-PatchSet: 2 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Roy Golan <[email protected]> Gerrit-Reviewer: Michael Kublin <[email protected]> Gerrit-Reviewer: Roy Golan <[email protected]> _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
