Juan Hernandez has posted comments on this change.
Change subject: bootstrap: detach OVirtUpgrader from VdsInstaller into
OVirtNodeUpgrade
......................................................................
Patch Set 12: (12 inline comments)
See my comments inside. Nothing new.
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/InstallerMessages.java
Line 27: public void post(Severity severity, String text) {
Line 28: AuditLogType logType;
Line 29: AuditLogableBase logable = new AuditLogableBase(_vds.getId());
Line 30: logable.AddCustomValue("Message", text);
Line 31: switch (severity) {
You are still not using the same formatting that the rest of the code. The
"case" should be aligned with the "switch".
Line 32: case INFO:
Line 33: logType = AuditLogType.VDS_INSTALL_IN_PROGRESS;
Line 34: log.infoFormat("Installation {0}: {1}",
_vds.gethost_name(), text);
Line 35: break;
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/OVirtNodeUpgrade.java
Line 27:
Line 28: private static final int BUFFER_SIZE = 10 * 1024;
Line 29: private static final int THREAD_JOIN_TIMEOUT = 20 * 1000;
Line 30:
Line 31: private static Log log = LogFactory.getLog(OVirtNodeUpgrade.class);
This ^ should be final.
Line 32:
Line 33: private SSHDialog.Control _control;
Line 34: private Thread _thread;
Line 35: private EngineSSHDialog _dialog;
Line 45: /**
Line 46: * Set vds object status.
Line 47: * @param status new status.
Line 48: *
Line 49: * For this simple task, no need to go via command mechanism.
The description should go before the parameters.
Line 50: */
Line 51: private void _setVdsStatus(VDSStatus status) {
Line 52: _vds.setstatus(status);
Line 53:
Line 116: */
Line 117: @Override
Line 118: public void finalize() {
Line 119: close();
Line 120: }
Still using the finalizer.
Line 121:
Line 122: /**
Line 123: * Free resources.
Line 124: */
....................................................
File
backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/ssh/EngineSSHDialog.java
Line 59: */
Line 60: public void useDefaultKeyPair() throws KeyStoreException {
Line 61: String alias = Config.<String>GetValue(ConfigValues.CertAlias);
Line 62: String p12 = Config.<String>GetValue(ConfigValues.keystoreUrl);
Line 63: char[] password =
Config.<String>GetValue(ConfigValues.keystorePass).toCharArray();
These ^ could be final.
Line 64:
Line 65: KeyStore.PrivateKeyEntry entry;
Line 66: InputStream in = null;
Line 67: try {
....................................................
File
backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/ssh/SSHDialog.java
Line 58: * Set streams to process.
Line 59: * @param incoming incoming stream.
Line 60: * @param outgoing outgoing stream.
Line 61: *
Line 62: * Streams are null when sink is removed from session.
Description should go before the parameters.
Line 63: */
Line 64: public void setStreams(InputStream incoming, OutputStream
outgoing);
Line 65:
Line 66: /**
Line 103: */
Line 104: @Override
Line 105: public void finalize() {
Line 106: disconnect();
Line 107: }
No finalizers, please.
Line 108:
Line 109: /**
Line 110: * Get session public key.
Line 111: * @return public key or null.
Line 165: * Set password to use.
Line 166: * @param password.
Line 167: *
Line 168: * If both password and key pair are set key pair
Line 169: * is used.
Description before parameters.
Line 170: */
Line 171: public void setPassword(String password) {
Line 172: _password = password;
Line 173: }
Line 176: * Set key pair.
Line 177: * @param keyPair key pair.
Line 178: *
Line 179: * If both password and key pair are set key pair
Line 180: * is used.
Description before parameters.
Line 181: */
Line 182: public void setKeyPair(KeyPair keyPair) {
Line 183: _keyPair = keyPair;
Line 184: }
Line 186: /**
Line 187: * Set soft timeout.
Line 188: * @param timeout timeout.
Line 189: *
Line 190: * Soft timeout is reset when there is session activity.
Same.
Line 191: */
Line 192: public void setSoftTimeout(long timeout) {
Line 193: _softTimeout = timeout;
Line 194: }
Line 196: /**
Line 197: * Set hard timeout.
Line 198: * @param timeout timeout.
Line 199: *
Line 200: * Hard timeout is maximum duration of session.
Same.
Line 201: */
Line 202: public void setHardTimeout(long timeout) {
Line 203: _hardTimeout = timeout;
Line 204: }
Line 292: }
Line 293: else {
Line 294: stdinList = new
LinkedList<InputStream>(Arrays.asList(initial));
Line 295: }
Line 296: stdinList.add(pinStdin);
Using an array list is more efficent, and you know the correct size, I don't
understand why you insist in using LinkedList.
Line 297:
Line 298: try {
Line 299: sink.setControl(
Line 300: new Control() {
--
To view, visit http://gerrit.ovirt.org/9174
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iff19fdb9f717d424f23bc5d4e5a8df8fce8a58bf
Gerrit-PatchSet: 12
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Alon Bar-Lev <[email protected]>
Gerrit-Reviewer: Allon Mureinik <[email protected]>
Gerrit-Reviewer: Alon Bar-Lev <[email protected]>
Gerrit-Reviewer: Barak Azulay <[email protected]>
Gerrit-Reviewer: Doron Fediuck <[email protected]>
Gerrit-Reviewer: Douglas Schilling Landgraf <[email protected]>
Gerrit-Reviewer: Juan Hernandez <[email protected]>
Gerrit-Reviewer: Shireesh Anjal <[email protected]>
Gerrit-Reviewer: Yair Zaslavsky <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
