Allon Mureinik has posted comments on this change.
Change subject: core: libosinfo - introduce libosinfo service
......................................................................
Patch Set 8: I would prefer that you didn't submit this
(10 inline comments)
-1 given since the tests won't pass without libosinfo's xmls installed.
otherwise, the code looks fine, except for some minor inline issues.
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/CpuArch.java
Line 9: ALL(1),
Line 10: i386(2),
Line 11: i586(3),
Line 12: i686(4),
Line 13: X86_64(5);
moreover - the ID seems to be identical to the ordinal, so it seems redeundent.
Line 14:
Line 15: private final int id;
Line 16:
Line 17: private static Map<Integer, CpuArch> map = new HashMap<Integer,
CpuArch>(values().length);
....................................................
File backend/manager/modules/services/libosinfo/interface/pom.xml
Line 26: <dependency>
Line 27: <groupId>${engine.groupId}</groupId>
Line 28: <artifactId>utils</artifactId>
Line 29: <version>${engine.version}</version>
Line 30: </dependency>
I'd a comment and some white spaces before this one to distinguish between
production code dependencies and testing dependencies.
Line 31: <dependency>
Line 32: <groupId>${engine.groupId}</groupId>
Line 33: <artifactId>utils</artifactId>
Line 34: <version>${engine.version}</version>
....................................................
File
backend/manager/modules/services/libosinfo/interface/src/main/java/org/ovirt/engine/core/services/libosinfo/LibosinfoService.java
Line 19: long getRecommendedRam(String osId, CpuArch cpuArch);
Line 20:
Line 21: /**
Line 22: * @param shortId
Line 23: * @return an Os instance for the given shortId or an empty Os
instance (most String values with "") if there is no
this line seems a bit long.
Perhaps add a "\n" somewhere in the middle there?
Line 24: * match.
Line 25: */
Line 26: Os getByShortId(String shortId);
Line 27:
....................................................
File
backend/manager/modules/services/libosinfo/interface/src/main/java/org/ovirt/engine/core/services/libosinfo/LibosinfoServiceXmlImpl.java
Line 28: private Map<String, Os> osInfoMap = new HashMap<String, Os>();
Line 29:
Line 30: private static final ObjectFactory factory = new ObjectFactory();
Line 31: private static final Os emptyOs = factory.createOs();
Line 32: private static final Os.Resources emptyResource =
factory.createOsResources();
please use constant naming conventions (e.g., EMPTY_OS)
Line 33:
Line 34: private LibosinfoServiceXmlImpl() {
Line 35: init();
Line 36: }
Line 34: private LibosinfoServiceXmlImpl() {
Line 35: init();
Line 36: }
Line 37:
Line 38: public void init() {
why is this public?
Line 39: emptyOs.setFamily("");
Line 40: emptyOs.setId("");
Line 41: emptyOs.setShortId("");
Line 42: emptyOs.setName("");
Line 45: Recommended rec = factory.createOsResourcesRecommended();
Line 46: min.setCpu(-1);
Line 47: min.setRam(-1);
Line 48: rec.setCpu(-1);
Line 49: rec.setRam(-1l);
use -1L instead of -1l.
There's less of a chance to misread it as -11 ;-)
Line 50: emptyResource.setArch("");
Line 51: emptyResource.setMinimum(min);
Line 52: emptyResource.setRecommended(rec);
Line 53: emptyOs.getResources().add(emptyResource);
Line 61: if (f.getName().endsWith(".xml")) {
Line 62: for (Os os : ((Libosinfo)
unmarshaller.unmarshal(f)).getOs()) {
Line 63: osInfoMap.put(os.getShortId(), os);
Line 64: }
Line 65: }
I'd add something down the lines of
else {
log.debug ("enounterred " + f.getName() + " in libosinfo dir, ignoring");
}
Line 66: }
Line 67:
Line 68: } catch (JAXBException e) {
Line 69: // failed to load files or unmarshal objects from them
Line 128: }
Line 129:
Line 130: @Override
Line 131: public Set<String> getShortIds() {
Line 132: return osInfoMap.keySet();
perhaps use Collections.unmodifiableSet() ?
Line 133: }
Line 134:
....................................................
File
backend/manager/modules/services/libosinfo/interface/src/test/java/org/ovirt/engine/core/services/libosinfo/LibosinfoServiceTest.java
Line 13:
Line 14:
Line 15: @Rule
Line 16: public static MockConfigRule mcr = new
MockConfigRule(MockConfigRule.mockConfig(ConfigValues.LibosinfoOsDataDir,
Line 17: "/usr/share/libosinfo/data/oses"));
If whoever build the engine (i.e., runs the test) does not have libosinfo
installed the test will fail.
I'd copy some (all?) of the XMLs to src/test/reousrces, and read it from there.
Line 18:
Line 19: private LibosinfoService service;
Line 20:
Line 21: @Before
Line 27: }
Line 28:
Line 29: @Test
Line 30: public void testRecommendedCpu() {
Line 31:
Assert.assertTrue(service.getRecommendedCpu(VmOsType.Windows2003.name(),
CpuArch.i386) > 100);
why not statically import Assert.assertTrue?
Line 32: }
Line 33:
Line 34: @Test
Line 35: public void testRecommendedRam() {
--
To view, visit http://gerrit.ovirt.org/8472
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I663dbae36d583ca4c524b2957a9a695e44207975
Gerrit-PatchSet: 8
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Roy Golan <[email protected]>
Gerrit-Reviewer: Allon Mureinik <[email protected]>
Gerrit-Reviewer: Alon Bar-Lev <[email protected]>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <[email protected]>
Gerrit-Reviewer: Michal Skrivanek <[email protected]>
Gerrit-Reviewer: Omer Frenkel <[email protected]>
Gerrit-Reviewer: Roy Golan <[email protected]>
Gerrit-Reviewer: Yair Zaslavsky <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
