Piotr Kliczewski has posted comments on this change.
Change subject: [wip] engine: Json-rpc integration
......................................................................
Patch Set 5:
(17 comments)
@Alon - Thank you for creating repo for me but I want to focus on working API
and then will move the code to you project. It is a matter of priorities.
Please take a look at TODO list in the commit message.
....................................................
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;
Agree. Will change.
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())) {
We do not use binary. Good catch :). Will change it.
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())) {
Optional parameter means that it is not required from vdsm api perspective.
There are some commands in the engine which send a value or sometimes it is
empty. Here I check whether it is empty and do not pollute json message with no
value.
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;
Done
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) {
Here you can have number of request sent in batch so the original idea was to
track all of them. I can see that this particular piece of code was not well
refactored :). I removed notifications from original code. Let me rework this
code.
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 {
Agree and will add java docs :).
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) {
There idea is to batch requests so feel free to call it more then once.
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");
We have one worker per connection so it would be good to come up with good
naming scheme here.
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: }
Parsing of the response should not close the connection. I will rework this as
part of connection reconnect logic that is missing.
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;
it is a thread so in principal it should be volatile when you exchange
information to have values copied immediately to other threads memory. We are
using blocking queue for information exchange so we will be notified when there
are new values no matter how long it takes to copy the value from other parts
of memory.
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 {
It is neat trick to use only try block and have the lock unlock by autoClosing.
Please take a look at ChainedOperation class. It is only for simplicity of the
code.
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<>();
Good point. It is leftover from the old code. Will change this.
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 {
It is not used and needs to be removed.
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) {
it should be used blocking queue here. Leftovers in tests which was not really
well refactored.
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,
Currently when you host deploy the json port is not opened so you need to go
with uncommenting in manually when you have the host configured with json. I
will be uncommented when we provide UI check box and fix host deploy.
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 answer as before
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> {
Will update java docs.
The idea is to have full async behavior in the code so any call returns a map
without waiting on reply from vdsm. You can pass around this object and it
won't be populated unless you call one of the methods to get the values. In
this situation you may have no wait time when you call get value since the
value was updated for you in the background.
The engine code asks for value in any of *ReturnForXmlRpc classes. When you
change this behavior to be async and get the value when it is needed you will
get rid of wait time for the response which will speed up the 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 <[email protected]>
Gerrit-Reviewer: Alon Bar-Lev <[email protected]>
Gerrit-Reviewer: Barak Azulay <[email protected]>
Gerrit-Reviewer: Eli Mesika <[email protected]>
Gerrit-Reviewer: Piotr Kliczewski <[email protected]>
Gerrit-Reviewer: Saggi Mizrahi <[email protected]>
Gerrit-Reviewer: Yair Zaslavsky <[email protected]>
Gerrit-Reviewer: mooli tayer <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
