Yair Zaslavsky has posted comments on this change.
Change subject: [wip] engine: Json-rpc integration
......................................................................
Patch Set 5:
(17 comments)
....................................................
File
backend/manager/modules/jsonrpc/src/main/java/org/ovirt/engine/jsonrpc/client/JsonRpcClient.java
Line 6: import java.util.concurrent.ConcurrentHashMap;
Line 7: import java.util.concurrent.ConcurrentMap;
Line 8: import java.util.concurrent.Future;
Line 9:
Line 10: import javax.management.openmbean.KeyAlreadyExistsException;
Why do you need this exception? I would prefer to use our own here, and not
something that might confuse the developer that might ask if our code relates
to jmx and mbeans.
Line 11:
Line 12: import org.codehaus.jackson.JsonEncoding;
Line 13: import org.codehaus.jackson.JsonGenerator;
Line 14: import org.codehaus.jackson.JsonNode;
....................................................
File
backend/manager/modules/jsonrpc/src/main/java/org/ovirt/engine/jsonrpc/client/JsonRpcError.java
Line 45: }
Line 46:
Line 47: String message = null;
Line 48: tmp = node.get("message");
Line 49: if ((tmp != null) && (tmp.isTextual())) {
is there any other option for a message as far as you know - that is, message
that is not textual?
Line 50: message = tmp.asText();
Line 51: }
Line 52:
Line 53: final JsonNode data = node.get("data");
....................................................
File
backend/manager/modules/jsonrpc/src/main/java/org/ovirt/engine/jsonrpc/client/RequestBuilder.java
Line 28: return this;
Line 29: }
Line 30:
Line 31: public RequestBuilder withOptionalParameter(String name, String
value) {
Line 32: if (value != null && !"".equals(value.trim())) {
So an optional parameter means it's not null or empty or is this just one of
the characteristics of optional parameter?
Can you please elaborate here?
Line 33: return withParameter(name, value);
Line 34: }
Line 35: return this;
Line 36: }
....................................................
File
backend/manager/modules/jsonrpc/src/main/java/org/ovirt/engine/jsonrpc/client/ResponseDecomposer.java
Line 13:
Line 14: public class ResponseDecomposer {
Line 15: private static Log log =
LogFactory.getLog(ResponseDecomposer.class);
Line 16: private JsonRpcResponse response;
Line 17: private ObjectMapper mapper;
Is ObjectMapper stateless (hence thread safe) by nature?
Can we change this to
private static ObjectMapper mapper ?
Line 18:
Line 19: public ResponseDecomposer(JsonRpcResponse response) {
Line 20: this.response = response;
Line 21: this.mapper = new ObjectMapper();
....................................................
File
backend/manager/modules/jsonrpc/src/main/java/org/ovirt/engine/jsonrpc/client/internal/BatchCall.java
Line 25: }
Line 26: i++;
Line 27: }
Line 28: responses = new ArrayList<>(i);
Line 29: if (i == 0) {
Not sure exactly what you tried to achieve here with the if?
Can't I initialize CountdownLatch with 0?
Line 30: latch = new CountDownLatch(1);
Line 31: latch.countDown();
Line 32: } else {
Line 33: latch = new CountDownLatch(i);
....................................................
File
backend/manager/modules/jsonrpc/src/main/java/org/ovirt/engine/jsonrpc/client/internal/Call.java
Line 9:
Line 10: import org.ovirt.engine.jsonrpc.client.JsonRpcRequest;
Line 11: import org.ovirt.engine.jsonrpc.client.JsonRpcResponse;
Line 12:
Line 13: public class Call implements Future<JsonRpcResponse>, JsonRpcCall {
General comment - not enough documentation at code, so it's also hard to ask
questions...
Line 14:
Line 15: private final BatchCall batchCall;
Line 16: private final boolean notification;
Line 17:
Line 25: return batchCall.cancel(mayInterruptIfRunning);
Line 26: }
Line 27:
Line 28: @Override
Line 29: public void addResponse(JsonRpcResponse response) {
Is Call meant to handle single calls? if so - what happens if I call twice to
addResponse?
Line 30: batchCall.addResponse(response);
Line 31: }
Line 32:
Line 33: private JsonRpcResponse extractResponse(List<JsonRpcResponse>
list) {
....................................................
File
backend/manager/modules/jsonrpc/src/main/java/org/ovirt/engine/jsonrpc/client/internal/ResponseWorker.java
Line 27: }
Line 28:
Line 29: public ResponseWorker() {
Line 30: this.queue = new LinkedBlockingQueue<>();
Line 31: setName("ResponseWorker");
Can we have only one response worker in the system?
Line 32: setDaemon(true);
Line 33: start();
Line 34: }
Line 35:
Line 67: }
Line 68: } catch (Exception e) {
Line 69: ctx.client.close();
Line 70: continue;
Line 71: }
finally block with ctx.client.close ?
Can we skip it?
Line 72: }
Line 73: }
Line 74:
Line 75: private void processIncomingObject(JsonRpcClient client, JsonNode
node) {
....................................................
File
backend/manager/modules/jsonrpc/src/main/java/org/ovirt/engine/jsonrpc/client/reactors/ReactorFactory.java
Line 8: public class ReactorFactory {
Line 9:
Line 10: private static NioReactor nioReactor;
Line 11: private static SSLReactor sslReactor;
Line 12: private static ResponseWorker worker;
shouldn't this be volatile?
Line 13:
Line 14: public static Reactor getReactor(ManagerProvider provider) throws
IOException, GeneralSecurityException {
Line 15: if (provider == null) {
Line 16: return getNioReactor();
....................................................
File
backend/manager/modules/jsonrpc/src/main/java/org/ovirt/engine/jsonrpc/client/utils/LockWrapper.java
Line 1: package org.ovirt.engine.jsonrpc.client.utils;
Line 2:
Line 3: import java.util.concurrent.locks.Lock;
Line 4:
Line 5: public class LockWrapper implements AutoCloseable {
I think there are other places in code where you can think of implementing
wrappers for auto closable.
In addition, you can use the try .. finally blocks for lock and unlock .. why
do u need an autoclosable wrapper for lock?
Line 6:
Line 7: private Lock lock;
Line 8:
Line 9: public LockWrapper(Lock lock) {
....................................................
File
backend/manager/modules/jsonrpc/src/test/java/org/ovirt/engine/jsonrpc/server/JsonRpcServer.java
Line 26: private Selector selector;
Line 27:
Line 28: private Worker worker;
Line 29:
Line 30: private List<ChangeRequest> pendingChanges = new LinkedList<>();
maybe use some other lock and not synchronized? will this be more beneficial?
Line 31:
Line 32: private Map<SocketChannel, List<ByteBuffer>> pendingData = new
HashMap<>();
Line 33: private boolean isDone = true;
Line 34:
Line 214:
Line 215: return socketSelector;
Line 216: }
Line 217:
Line 218: public static void main(String[] args) throws IOException {
Should not be here, use test classes.
Line 219: new JsonRpcServer(9090);
Line 220: }
....................................................
File
backend/manager/modules/jsonrpc/src/test/java/org/ovirt/engine/jsonrpc/server/Worker.java
Line 50:
Line 51: public void run() {
Line 52: ServerDataEvent dataEvent;
Line 53: while (true) {
Line 54: synchronized (queue) {
why not use blocking queue provided by java concurrent package?
Line 55: while (queue.isEmpty()) {
Line 56: try {
Line 57: queue.wait();
Line 58: } catch (InterruptedException e) {
....................................................
File
backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/VdsManager.java
Line 192: int clientRetries =
Config.<Integer>GetValue(ConfigValues.vdsRetries);
Line 193:
Line 194: // TODO use input from the UI whether to go with xml or json
Line 195: // _vdsProxy = new
JsonRpcVdsServer(JsonRpcUtils.createClient(_vds.getHostName(),
Line 196: // /*_vds.getPort()*/ 4044, connectionTimeOut,
why is this in comment?
your patch should assume that you do want to use json rpc, right?
Line 197: // /*Config.<Boolean>
GetValue(ConfigValues.EncryptHostCommunication)*/ false), clientTimeOut,
clientRetries);
Line 198:
Line 199: Pair<VdsServerConnector, HttpClient> returnValue =
Line 200: XmlRpcUtils.getConnection(_vds.getHostName(),
....................................................
File
backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/irsbroker/IrsBrokerCommand.java
Line 541: int connectionTimeOut =
Config.<Integer>GetValue(ConfigValues.vdsConnectionTimeout) * 1000;
Line 542: int clientRetries = Config.<Integer>
GetValue(ConfigValues.vdsRetries);
Line 543:
Line 544: // TODO check whether the host should use xml
or json based on UI choice
Line 545: // privatemIrsProxy = new
JsonRpcIIrsServer(JsonRpcUtils.createClient(host,
same question as before - if not needed, considerto remove this from code.
Line 546: // /*_vds.getPort()*/ 4044,
connectionTimeOut,
Line 547: // /*Config.<Boolean>
GetValue(ConfigValues.EncryptHostCommunication)*/ false), clientTimeOut,
clientRetries);
Line 548: Pair<IrsServerConnector, HttpClient>
returnValue =
Line 549: XmlRpcUtils.getConnection(host,
....................................................
File
backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/jsonrpc/FutureMap.java
Line 12: import org.ovirt.engine.jsonrpc.client.JsonRpcResponse;
Line 13: import org.ovirt.engine.jsonrpc.client.ResponseDecomposer;
Line 14:
Line 15: @SuppressWarnings("serial")
Line 16: public class FutureMap implements Map<String, Object> {
Had hard time to understand why you need it - once again - general comment
about lack of comments in code.
Line 17: private final static String STATUS = "status";
Line 18: private final static String DEFAULT_KEY = "info";
Line 19: private Future<JsonRpcResponse> response;
Line 20: private Map<String, Object> responseMap = new HashMap<String,
Object>();
--
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: 5
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: 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
