Piotr Kliczewski has posted comments on this change. Change subject: core: protocol fall back for older vdsms ......................................................................
Patch Set 2: (4 comments) @Alon - Please be more constructive and give a bit more details. I am happy to discuss any decisions that were made and fix them if necessary. The code behaves as you described but this is very specific scenario as pointed in commit message. We want to improve usability when the user makes a mistake. http://gerrit.ovirt.org/#/c/33728/2/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/transport/ProtocolDetector.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/transport/ProtocolDetector.java: Line 36: * Attempts to connect to vdsm using a proxy from {@code VdsManager} for a host. Line 37: * Line 38: * @return <code>true</code> if connected or <code>false</code> if connection failed. Line 39: */ Line 40: public boolean attemptConnection() { > so if we immediately do a Future.get() then your blocking on it. what's th The call is async but we want to get the response in sync way. In this case we are interested in having the response as soon as possible because we are not sure that we will connect using current protocol. Line 41: try { Line 42: long timeout = Config.<Integer> getValue(ConfigValues.SetupNetworksPollingTimeout); Line 43: FutureVDSCall<VDSReturnValue> task = Line 44: Backend.getInstance().getResourceManager().runFutureVdsCommand(FutureVDSCommandType.TimeBoundPoll, Line 47: task.get(timeout, TimeUnit.SECONDS); Line 48: Line 49: if (returnValue.getSucceeded()) { Line 50: return true; Line 51: } > I think Timeout exceptions are converted a return value but I'm not sure. Sure I will check. Line 52: } catch (TimeoutException ignored) { Line 53: } Line 54: return false; Line 55: } Line 80: vdsStatic.setProtocol(VdsProtocol.XML); Line 81: TransactionSupport.executeInNewTransaction(new TransactionMethod<Void>() { Line 82: @Override Line 83: public Void runInTransaction() { Line 84: DbFacade.getInstance().getVdsStaticDao().update(vdsStatic); > is that have to be committed what so ever? Yes. When we detect that we talk to old vdsm and we are able only to use xmlrpc we need to commit it. The host will move to Non-operational stating that its version do not match cluster compatibility version. The user can move it to different cluster or change the cluster version so it needs to be usable after those operations. Line 85: return null; Line 86: } Line 87: }); Line 88: } http://gerrit.ovirt.org/#/c/33728/2/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/TimeBoundPollVDSCommand.java File backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/TimeBoundPollVDSCommand.java: Line 11: */ Line 12: @Logged(executionLevel = LogLevel.TRACE, errorLevel = LogLevel.DEBUG) Line 13: public class TimeBoundPollVDSCommand<P extends TimeBoundPollVDSCommandParameters> extends FutureVDSCommand<P> { Line 14: Line 15: private long timeout; > I guess we can just use the params directly instead local vars? Done Line 16: private TimeUnit unit; Line 17: Line 18: public TimeBoundPollVDSCommand(P parameters) { Line 19: super(parameters); -- To view, visit http://gerrit.ovirt.org/33728 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie6f48bec60b520c089f326f8c5e79aec288ff3d6 Gerrit-PatchSet: 2 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: Oved Ourfali <[email protected]> Gerrit-Reviewer: Piotr Kliczewski <[email protected]> Gerrit-Reviewer: Roy Golan <[email protected]> Gerrit-Reviewer: Saggi Mizrahi <[email protected]> Gerrit-Reviewer: [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
