Juan Hernandez has posted comments on this change.
Change subject: bootstrap: detach OVirtUpgrader from VdsInstaller into
OVirtNodeUpgrade
......................................................................
Patch Set 10: (10 inline comments)
Regarding the _ and s_, do you expect to maintain this code alone? If not I
think you should take into consideration the opinions of the reviewers and
adapt to what they suggest and is common practice when developing in Java.
I have to say that I feel a bit offended when you say that I have to open my
mind. I consider myself open minded, and I think this has nothing to do with
that. Did you ask yourself if you are being open minded in this particular
subject? It looks to me that instead you want to force others to use what you
like or are used to.
Apart from that the code in this patch looks good. See my responses inside.
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/InstallerMessages.java
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;
Line 36: default:
Then better put the default at the end and throw an exception or log an error.
Remember to fix the formatting of the switch statement.
Line 37: case WARNING:
Line 38: logable.setCustomId(_sequence++);
Line 39: logType = AuditLogType.VDS_INSTALL_IN_PROGRESS_WARNING;
Line 40: log.warnFormat("Installation {0}: {1}",
_vds.gethost_name(), text);
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/InstallVdsCommand.java
Line 75: return result;
Line 76: }
Line 77:
Line 78: @Override
Line 79: protected void executeCommand() {
No, not sure.
Line 80: if (
Line 81: getVds() != null &&
Line 82: isOvirtReInstallOrUpgrade()
Line 83: ) {
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/OVirtNodeUpgrade.java
Line 22:
Line 23: /**
Line 24: * ovirt-node upgrade.
Line 25: */
Line 26: public class OVirtNodeUpgrade implements SSHDialog.Sink, Runnable {
You need a runnable, but you don't need to implement it in the class. Either
use an anonymous class:
Runnable theRunnable = new Runnable() {
public void run() {
...
}
};
Thread myThread = new Thread(theRunnable, "My thread");
Or override the "run" method of the Thread class:
Thread myThread = new Tread("MyThread") {
public void run() {
...
}
};
I prefer the first.
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 79: */
Line 80: @Override
Line 81: public void finalize() {
Line 82: close();
Line 83: }
Use of finalizers is a bad practice, as it has been discussed before, so the
sooner we stop using them the better. We should not use them more and more, but
less and less.
Line 84:
Line 85: /**
Line 86: * Free resources.
Line 87: */
Line 84:
Line 85: /**
Line 86: * Free resources.
Line 87: */
Line 88: public void close() {
Good, so there is no need for the finalizer.
Line 89: stop();
Line 90: if (_dialog != null) {
Line 91: _dialog.disconnect();
Line 92: _dialog = null;
....................................................
File
backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/ssh/EngineSSHDialog.java
Line 70:
Line 71: entry = (KeyStore.PrivateKeyEntry)ks.getEntry(
Line 72: alias,
Line 73: new KeyStore.PasswordProtection(
Line 74: password.toCharArray()
Use a local variable to store the resulting array of characters (once) and then
clean it when done.
Line 75: )
Line 76: );
Line 77: }
Line 78: catch (Exception e) {
....................................................
File
backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/ssh/SSHDialog.java
Line 103: */
Line 104: @Override
Line 105: public void finalize() {
Line 106: disconnect();
Line 107: }
Same answer as previous and past discussion as well. The fact that you used
this once is no excuse to continue using this bad practice.
Line 108:
Line 109: /**
Line 110: * Get session public key.
Line 111: * @return public key or null.
Line 226: _port,
Line 227: _hardTimeout,
Line 228: _softTimeout
Line 229: )
Line 230: );
Then you may want to improve it.
Line 231:
Line 232: try {
Line 233: if (_client != null) {
Line 234: throw new IOException("Already connected");
Line 289: stdinList = new LinkedList<InputStream>();
Line 290: }
Line 291: else {
Line 292: stdinList = new
LinkedList<InputStream>(Arrays.asList(initial));
Line 293: }
ArrayList is lighter if you know the number of elements in advance (and even if
you don't for most cases), as it allocates only one object in memory, as
opposed to as many objects as elements in the list.
Line 294: stdinList.add(pinStdin);
Line 295:
Line 296: try {
Line 297: sink.setControl(
....................................................
File
backend/manager/modules/utils/src/test/java/org/ovirt/engine/core/utils/ssh/SSHDialogTest.java
Line 22: import javax.naming.TimeLimitExceededException;
Line 23:
Line 24: import static org.junit.Assert.assertEquals;
Line 25: import static org.junit.Assert.assertTrue;
Line 26: import static org.junit.Assume.assumeTrue;
It is just common practice. Next time the list of imports is sorted (using
Eclipse organize imports, for example) it will automatically go to the
beginning of the class.
Line 27: import org.junit.After;
Line 28: import org.junit.AfterClass;
Line 29: import org.junit.Before;
Line 30: import org.junit.BeforeClass;
--
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: 10
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
