Yair Zaslavsky has posted comments on this change. Change subject: jsonrpc: Stomp changes in vdsbroker ......................................................................
Patch Set 13: (4 comments) http://gerrit.ovirt.org/#/c/26783/13/backend/manager/dependencies/src/main/modules/org/ovirt/vdsm-jsonrpc-java/main/module.xml File backend/manager/dependencies/src/main/modules/org/ovirt/vdsm-jsonrpc-java/main/module.xml: Line 9: <dependencies> Line 10: <module name="javax.api"/> Line 11: <module name="org.apache.commons.logging"/> Line 12: <module name="org.codehaus.jackson.jackson-mapper-asl"/> Line 13: <module name="org.codehaus.jackson.jackson-core-asl"/> can you please sort the module names? Line 14: </dependencies> Line 15: http://gerrit.ovirt.org/#/c/26783/13/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/jsonrpc/FutureMap.java File backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/jsonrpc/FutureMap.java: Line 40: super.put("message", "Done"); Line 41: super.put("code", 0); Line 42: } Line 43: }; Line 44: private Class<?> clazz = new HashMap<String, Object>().getClass(); does this really have a meaning, I mean, why not HashMap.class()? isnt type erasure working here as well? Line 45: private Class<?> subTypeClazz; Line 46: private boolean ignoreResponseKey = false; Line 47: Line 48: /** http://gerrit.ovirt.org/#/c/26783/13/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/jsonrpc/JsonRpcVdsServer.java File backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/jsonrpc/JsonRpcVdsServer.java: Line 1191: String volumeName, Line 1192: String brickName, Line 1193: String volumeStatusOption) { Line 1194: JsonRpcRequest request = Line 1195: new RequestBuilder("GlusterVolume.status").withParameter("volumeName", volumeName) HI, maybe worth having all this strings in the future as constants? Line 1196: .withParameter("brick", brickName) Line 1197: .withParameter("statusOption", volumeStatusOption) Line 1198: .build(); Line 1199: Map<String, Object> response = http://gerrit.ovirt.org/#/c/26783/13/backend/manager/modules/vdsbroker/src/test/java/org/ovirt/engine/core/vdsbroker/jsonrpc/JsonRpcIntegrationTest.java File backend/manager/modules/vdsbroker/src/test/java/org/ovirt/engine/core/vdsbroker/jsonrpc/JsonRpcIntegrationTest.java: Line 14: Line 15: @Ignore Line 16: public class JsonRpcIntegrationTest { Line 17: Line 18: private final static String HOST_ADDRESS = "192.168.1.10"; > Fine by me. Just add some comment clarifying that. you can have IMHO a similar mechanism here like with the jdbc.properties (i.e - have a resources file at test , and by default have this test shutdown ) and maybe add a profile for testing this specific test at maven, of course @Ignore is the quickest, just a thought for the future. Line 19: private final static int PORT = 4044; Line 20: private final static int TIMEOUT = 5000; Line 21: Line 22: @Test -- To view, visit http://gerrit.ovirt.org/26783 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: If78de6620ba6891543531ac8ddd633b67828a89c Gerrit-PatchSet: 13 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Piotr Kliczewski <piotr.kliczew...@gmail.com> Gerrit-Reviewer: Barak Azulay <bazu...@redhat.com> Gerrit-Reviewer: Daniel Erez <de...@redhat.com> Gerrit-Reviewer: Eli Mesika <emes...@redhat.com> Gerrit-Reviewer: Juan Hernandez <juan.hernan...@redhat.com> Gerrit-Reviewer: Oved Ourfali <oourf...@redhat.com> Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczew...@gmail.com> Gerrit-Reviewer: Saggi Mizrahi <smizr...@redhat.com> Gerrit-Reviewer: Yair Zaslavsky <yzasl...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
