Martin Peřina has posted comments on this change.
Change subject: engine: Draft - Json-rpc integration
......................................................................
Patch Set 7:
(15 comments)
....................................................
File
backend/manager/modules/jsonrpc/src/main/java/org/ovirt/engine/jsonrpc/client/JsonRpcClient.java
Line 39: private final ConcurrentMap<JsonNode, JsonRpcCall> runningCalls;
Line 40: private RetryPolicy policy = new DefaultClientRetryPolicy();
Line 41: private ConcurrentMap<JsonNode, ResponseTracking> map = new
ConcurrentHashMap<>();
Line 42: private Queue<JsonNode> queue = new ConcurrentLinkedQueue<>();
Line 43: private boolean isTracking = true;
Please move instance attribute initialization into constructor
Line 44:
Line 45: /**
Line 46: * Wraps {@link ReactorClient} to hide response update details.
Line 47: *
Line 163: removeRequestFromTracking(id);
Line 164: continue;
Line 165: }
Line 166: ResponseTracking tracking = map.get(id);
Line 167: if (new Date().getTime() >=
tracking.getTimeout()) {
Wouldn't it be better to call System.currentTimeMillis() so no new Date
instance is created?
Line 168: RetryContext context =
tracking.getContext();
Line 169: context.decreaseAttempts();
Line 170: if (context.getNumberOfAttempts() <= 0) {
Line 171: runningCalls.remove(id);
....................................................
File
backend/manager/modules/jsonrpc/src/main/java/org/ovirt/engine/jsonrpc/client/JsonRpcRequest.java
Line 8: * Java bean representation of the request.
Line 9: *
Line 10: */
Line 11: public class JsonRpcRequest {
Line 12: private static ObjectMapper mapper = new ObjectMapper();
final?
Line 13: private String method;
Line 14: private JsonNode params;
Line 15: private JsonNode id;
Line 16:
Line 66: "'jsonrpc' field missing in node");
Line 67: }
Line 68:
Line 69: String version = tmp.asText();
Line 70: if ((version == null) || (!version.equals("2.0"))) {
Please remove internal brackets:
if (version == null || !version.equals("2.0") {
I would also declare string "2.0" as some final static attribute.
Line 71: throw new IllegalArgumentException("Only jsonrpc 2.0 is
supported");
Line 72: }
Line 73:
Line 74: tmp = node.get("method");
....................................................
File
backend/manager/modules/jsonrpc/src/main/java/org/ovirt/engine/jsonrpc/client/JsonRpcResponse.java
Line 60: "'jsonrpc' field missing in node");
Line 61: }
Line 62:
Line 63: String version = jsonrpcNode.asText();
Line 64: if ((version == null) || (!version.equals("2.0"))) {
Same as in JsonRpcRequest
Line 65: throw new IllegalArgumentException("Only jsonrpc 2.0 is
supported");
Line 66: }
Line 67:
Line 68: final JsonNode id = node.get("id");
....................................................
File
backend/manager/modules/jsonrpc/src/main/java/org/ovirt/engine/jsonrpc/client/RequestBuilder.java
Line 32: * @param name - Name of the parameter.
Line 33: * @param value - Value of the parameter.
Line 34: * @return {@link RequestBuilder} to let add more parameters.
Line 35: */
Line 36: public RequestBuilder withParameter(String name, String value) {
Please rename to with()
Line 37: this.parameters.put(name, value);
Line 38: return this;
Line 39: }
Line 40:
Line 44: * @param name - Name of the parameter.
Line 45: * @param value - Value of the parameter or <code>null</code>.
Line 46: * @return {@link RequestBuilder} to let add more parameters.
Line 47: */
Line 48: public RequestBuilder withOptionalParameter(String name, String
value) {
Please rename to withOptional()
Line 49: if (value != null && !"".equals(value.trim())) {
Line 50: return withParameter(name, value);
Line 51: }
Line 52: return this;
Line 45: * @param value - Value of the parameter or <code>null</code>.
Line 46: * @return {@link RequestBuilder} to let add more parameters.
Line 47: */
Line 48: public RequestBuilder withOptionalParameter(String name, String
value) {
Line 49: if (value != null && !"".equals(value.trim())) {
Please use StringUtils.isNotEmpty()
Line 50: return withParameter(name, value);
Line 51: }
Line 52: return this;
Line 53: }
Line 61: * or empty <code>List</code>.
Line 62: * @return {@link RequestBuilder} to let add more parameters.
Line 63: */
Line 64: @SuppressWarnings("rawtypes")
Line 65: public RequestBuilder withOptionalParameterAsList(String name,
List value) {
Please rename to withOptionalList()
Line 66: if (value != null && value.size() > 0) {
Line 67: return withParameter(name, value);
Line 68: }
Line 69: return this;
Line 62: * @return {@link RequestBuilder} to let add more parameters.
Line 63: */
Line 64: @SuppressWarnings("rawtypes")
Line 65: public RequestBuilder withOptionalParameterAsList(String name,
List value) {
Line 66: if (value != null && value.size() > 0) {
Please use !value.isEmpty() instead of size checking
Line 67: return withParameter(name, value);
Line 68: }
Line 69: return this;
Line 70: }
Line 78: * or empty <code>Map</code>.
Line 79: * @return {@link RequestBuilder} to let add more parameters.
Line 80: */
Line 81: @SuppressWarnings("rawtypes")
Line 82: public RequestBuilder withOptionalParameterAsMap(String name, Map
value) {
Please rename to withOptionalMap()
Line 83: if (value != null && value.keySet().size() > 0) {
Line 84: return withParameter(name, value);
Line 85: }
Line 86: return this;
Line 79: * @return {@link RequestBuilder} to let add more parameters.
Line 80: */
Line 81: @SuppressWarnings("rawtypes")
Line 82: public RequestBuilder withOptionalParameterAsMap(String name, Map
value) {
Line 83: if (value != null && value.keySet().size() > 0) {
Please use !value.isEmpty() instead of size checking
Line 84: return withParameter(name, value);
Line 85: }
Line 86: return this;
Line 87: }
....................................................
File
backend/manager/modules/jsonrpc/src/main/java/org/ovirt/engine/jsonrpc/client/reactors/Reactor.java
Line 27: private static Log log = LogFactory.getLog(Reactor.class);
Line 28: private static final int TIMEOUT = 1000;
Line 29: private final AbstractSelector selector;
Line 30: private final ReactorScheduler scheduler;
Line 31: private boolean isRunning = false;
Please move non static initialization to constructor
Line 32:
Line 33: public Reactor() throws IOException {
Line 34: this.selector = SelectorProvider.provider().openSelector();
Line 35: this.scheduler = new ReactorScheduler();
....................................................
File
backend/manager/modules/jsonrpc/src/main/java/org/ovirt/engine/jsonrpc/client/utils/JsonUtils.java
Line 49: return os.toByteArray();
Line 50: }
Line 51:
Line 52: public static JsonRpcResponse
buildFailedResponse(JsonRpcRequest request) {
Line 53: String jsonMessge =
jsonMessage?
Line 54: "{\"jsonrpc\": \"2.0\", \"id\": \"" +
request.getPlainId() + "\", \"error\": {\"message\":"
Line 55: + " \"Message timeout which can be caused
by communication issues'\", \"code\": 5022}}";
Line 56: JsonRpcResponse response = null;
Line 57: try {
....................................................
File
backend/manager/modules/jsonrpc/src/main/java/org/ovirt/engine/jsonrpc/client/utils/ReactorScheduler.java
Line 21: }
Line 22:
Line 23: public void performPendingOperations() {
Line 24: boolean remove = false;
Line 25: for (int i = 0; i < +pendingOperations.size(); i++) {
Syntax error?
Line 26: Future<?> task = pendingOperations.peek();
Line 27: if (task instanceof FutureTask) {
Line 28: ((FutureTask<?>) task).run();
Line 29: remove = true;
--
To view, visit http://gerrit.ovirt.org/20926
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I66ef0c98f07de6c447a5fffc42c9dbc94580df46
Gerrit-PatchSet: 7
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Piotr Kliczewski <piotr.kliczew...@gmail.com>
Gerrit-Reviewer: Alon Bar-Lev <alo...@redhat.com>
Gerrit-Reviewer: Barak Azulay <bazu...@redhat.com>
Gerrit-Reviewer: Eli Mesika <emes...@redhat.com>
Gerrit-Reviewer: Martin Peřina <mper...@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: mooli tayer <mta...@redhat.com>
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
