Juan Hernandez has posted comments on this change.
Change subject: bootstrap: send complete bootstrap from engine
......................................................................
Patch Set 11: (15 inline comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VdsInstaller.java
Line 55: private Guid _fileGuid = new Guid();
Line 56: private final VDS _vds;
Line 57: private String serverInstallationTime;
Line 58:
Line 59: private static CachedTar s_bootstrapPackage;
"s_" for static private? No please.
Line 60: private String _bootstrapCommand;
Line 61:
Line 62: protected final VdsInstallerSSH _wrapper = new VdsInstallerSSH();
Line 63: private final OpenSslCAWrapper _caWrapper = new OpenSslCAWrapper();
Line 112: vds,
Line 113: Config.<String> GetValue(ConfigValues.BootstrapCommand)
Line 114: );
Line 115:
Line 116: if (s_bootstrapPackage == null) {
How many times do you want to create the CachedTar instance? In a
multi-threaded environment this can result in creating it many times. It is
better if you create in the initializer:
private static final bootstrapPackage = new CachedTar(...);
Line 117: String cache = System.getenv("ENGINE_CACHE");
Line 118: if (cache == null) {
Line 119: log.warn("ENGINE_CACHE environment not found using
tmpdir");
Line 120: cache = System.getProperty("java.io.tmpdir");
Line 121: }
Line 122: s_bootstrapPackage = new CachedTar(
Line 123: new File(cache, Config.<String>
GetValue(ConfigValues.BootstrapPackageName)),
Line 124: new File(Config.<String>
GetValue(ConfigValues.BootstrapPackageDirectory))
Line 125: );
These two files could be created inside the constructor of CachedTar, thus
simplifying this class, which is already quite long and complicated.
Line 126: }
Line 127:
Line 128: VdsGroupDAO vdsGroupDao =
DbFacade.getInstance().getVdsGroupDAO();
Line 129: Guid vdsGroupId = vds.getvds_group_id();
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/config/ConfigValues.java
Line 1430: BootstrapCommand(373),
Line 1431:
Line 1432: @TypeConverterAttribute(Integer.class)
Line 1433: @DefaultValueAttribute("10000")
Line 1434: BootstrapCacheRefreshInterval(374),
Is this parameter really needed? Can't we use just a constant?
Line 1435: @TypeConverterAttribute(String.class)
Line 1436: @DefaultValueAttribute("/usr/share/vdsm-bootstrap/interface-2")
Line 1437: BootstrapPackageDirectory(375),
Line 1438: @TypeConverterAttribute(String.class)
Line 1436: @DefaultValueAttribute("/usr/share/vdsm-bootstrap/interface-2")
Line 1437: BootstrapPackageDirectory(375),
Line 1438: @TypeConverterAttribute(String.class)
Line 1439: @DefaultValueAttribute("vdsm-bootstrap-2.tar")
Line 1440: BootstrapPackageName(376),
Is this parameter really needed? As the .tar file is created on the fly there
is no need to make it configurable.
Line 1441:
Line 1442: Invalid(65535);
Line 1443:
Line 1444: private int intValue;
....................................................
File backend/manager/modules/utils/pom.xml
Line 56: <dependency>
Line 57: <groupId>javatar</groupId>
Line 58: <artifactId>javatar</artifactId>
Line 59: <version>${javatar.version}</version>
Line 60: </dependency>
If you add the dependencies to the "dependencyManagement" section of the root
POM then there is no need to specify the version here.
Line 61:
Line 62: <dependency>
Line 63: <groupId>${engine.groupId}</groupId>
Line 64: <artifactId>engineencryptutils</artifactId>
....................................................
File
backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/hostinstall/CachedTar.java
Line 76: }
Line 77: }
Line 78: }
Line 79:
Line 80: private void ensure() throws IOException {
In a multithreaded environment this can be executed many times and thus the
.tar file can be written by several threads simultaneously. You need to put
some synchronization here.
Line 81: if (!this.archive.exists()) {
Line 82: log.info(
Line 83: String.format(
Line 84: "Tarball '%1$s' is missing, creating",
Line 81: if (!this.archive.exists()) {
Line 82: log.info(
Line 83: String.format(
Line 84: "Tarball '%1$s' is missing, creating",
Line 85: this.archive
Consider using "archive.getAbsolutePath()", that is very useful in log messages.
Line 86: )
Line 87: );
Line 88: this.nextCheckTime = System.currentTimeMillis() +
this.refreshInterval;
Line 89: create();
Line 93: if (archive.lastModified() <=
DirectoryTimestamp.getTimestamp(this.dir)) {
Line 94: log.info(
Line 95: String.format(
Line 96: "Tarball '%1$s' is out of date, re-creating",
Line 97: this.archive
Consider "archive.getAbsolutePath()".
Line 98: )
Line 99: );
Line 100: create();
Line 101: }
Line 119: /**
Line 120: * Get file of archive, without enforcing cache.
Line 121: * @return File name.
Line 122: *
Line 123: * This should be used only if file is not to be accessed
(messages).
This extra comment should go before the @return, otherwise it will appear as
part of the description of the @return when the javadoc is generated.
Line 124: */
Line 125: public File getFileNoUse() {
Line 126: return this.archive;
Line 127: }
Line 139: * Get stream of archive.
Line 140: * @return InputStream.
Line 141: */
Line 142: public InputStream getStream() throws IOException {
Line 143: return new FileInputStream(getFile());
You are opening the stream but not closing it. Add some text to the javadoc
comment explaining that the caller is responsible for closing the returned
stream.
You may consider removing this method, as it adds little for the caller, that
way there is no discussion of who should close the stream.
Line 144: }
....................................................
File
backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/hostinstall/DirectoryTimestamp.java
Line 21: * Returns the maximum timestamp of directory tree.
Line 22: * @param file directory/file name.
Line 23: * @return max timestamp.
Line 24: */
Line 25: public static long getTimestamp(File file) {
This kind of utility methods should probably go in the utils project. This one
looks like a good candidate for org.ovirt.engine.core.utils.FileUtil.
Line 26: if (file.isDirectory()) {
Line 27: long m = 0;
Line 28: for (String name : file.list()) {
Line 29: m = Math.max(m, getTimestamp(new File(file, name)));
....................................................
File
backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/hostinstall/Tar.java
Line 11: import org.apache.commons.logging.LogFactory;
Line 12:
Line 13: import com.ice.tar.InvalidHeaderException;
Line 14: import com.ice.tar.TarEntry;
Line 15: import com.ice.tar.TarOutputStream;
Any reason to use this particular implementation of tar? It didn't have updates
since 2003 and it is not available in some build systems.
Isn't it possible to do the same using a plain .zip file? That is natively
supported in Java, with no extra dependencies.
If you really need some of the tar specifics I would suggest to take a look at
apache-commons-compression, it looks healthier.
Line 16:
Line 17: /**
Line 18: * A simple recursive tar based on javatar.
Line 19: */
....................................................
File packaging/fedora/engine-service.py
Line 334: if os.path.exists(engineTmpDir):
Line 335: shutil.rmtree(engineTmpDir)
Line 336: os.mkdir(engineTmpDir)
Line 337: os.chown(engineTmpDir, engineUid, engineGid)
Line 338: if not os.path.exists(engineCacheDir):
Can you please add a line separator and a comment like:
# Create the cache directory if it doesn't exist:
Line 339: os.mkdir(engineCacheDir)
Line 340: os.chown(engineCacheDir, engineUid, engineGid)
Line 341:
Line 342: # Generate the main configuration from the template and copy it
to the
....................................................
File pom.xml
Line 45: <dozer.version>5.2.0</dozer.version>
Line 46: <cxf.version>2.2.7</cxf.version>
Line 47: <mina-core.version>2.0.1</mina-core.version>
Line 48: <sshd-core.version>0.7.0</sshd-core.version>
Line 49: <javatar.version>2.5</javatar.version>
If you add a dependency it should go in the "dependencyManagement" section, so
that there is no need to specify the version in the subprojects.
Line 50: <slf4j-jdk14.version>1.5.11</slf4j-jdk14.version>
Line 51: <gwt.version>2.2.0</gwt.version>
Line 52: <gwt.plugin.version>1.3.2.google</gwt.plugin.version>
Line 53: <findbugs.version>2.5.1</findbugs.version>
--
To view, visit http://gerrit.ovirt.org/6963
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6f4a09ca9e66f0c9f5f4f7b283a5f43986b7e603
Gerrit-PatchSet: 11
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Alon Bar-Lev <[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: Eyal Edri <[email protected]>
Gerrit-Reviewer: Juan Hernandez <[email protected]>
Gerrit-Reviewer: Ofer Schreiber <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
