Juan Hernandez has posted comments on this change. Change subject: core: CDI - EJB cleanup - Backend and Scheduler ......................................................................
Patch Set 2: (9 comments) Some comments inside, but all in all I think this change is very needed and should be merged as soon as possible. In fact I would even suggest to rebase it on top of the current master and make it the first in the CDI patch series. .................................................... File backend/manager/modules/bll/pom.xml Line 1: <project xmlns="http://maven.apache.org/POM/4.0.0"; xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"; xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd";> Now that we are importing javax.inject classes in the source code (or maybe we already did in previous patches) we should have an explicit dependency on the javax.inject:javax.inject artifact here. Line 2: <modelVersion>4.0.0</modelVersion> Line 3: Line 4: <parent> Line 5: <groupId>org.ovirt.engine.core</groupId> .................................................... File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/Backend.java Line 84 Line 85 Line 86 Line 87 Line 88 Isn't this interceptor or something equivalent ^ needed now? Line 81: Line 82: @Inject Line 83: private Log log; Line 84: @Inject Line 85: private SchedulerUtilQuartzImpl quartz; Shouldn't we use here the SchedulerUtil interface instead of the implementation? Line 86: Line 87: Line 88: public static BackendInternal getInstance() { Line 89: return EjbUtils.findBean(BeanType.BACKEND, BeanProxyType.LOCAL); .................................................... File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/InitBackendServicesOnStartupBean.java Line 36: @Inject Line 37: private Log log; Line 38: Line 39: @Inject Line 40: Backend backend; Shouldn't this be BackendLocal? I mean, the interface instead of the implementation? Can it be private? Line 41: Line 42: @Override Line 43: @PostConstruct Line 44: public void create() { .................................................... File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VdsEventListener.java Line 53: Line 54: public class VdsEventListener implements IVdsEventListener { Line 55: Line 56: @Inject Line 57: Backend backend; Interface instead of implementation? Can it be private? Line 58: Line 59: @Inject Line 60: private Log log; Line 61: .................................................... File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/adbroker/KerberosManager.java Line 30: @Inject Line 31: private Log log; Line 32: Line 33: @Inject Line 34: AppConfig config; I it possible to make this private? Line 35: Line 36: private boolean isKerberosAuth() { Line 37: boolean isKerberosAuth = false; Line 38: String authMethod = config.<String> getValue(ConfigValues.AuthenticationMethod); .................................................... File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/adbroker/UsersDomainsCacheManagerService.java Line 35: Line 36: @Inject Line 37: private Log log; Line 38: @Inject Line 39: AppConfig config; Can this be private? Line 40: private Map<String, Domain> domainsByName = new HashMap<String, Domain>(); Line 41: private Map<String, ConcurrentHashMap<String, UserDomainInfo>> domainsUsersInfoByUserNameAndDomainName = Line 42: new HashMap<String, ConcurrentHashMap<String, UserDomainInfo>>(); Line 43: private Map<String, ConcurrentHashMap<String, LdapGroup>> groupsPerDomain = .................................................... File backend/manager/modules/scheduler/pom.xml Line 32: Line 33: <dependency> Line 34: <groupId>javax.enterprise</groupId> Line 35: <artifactId>cdi-api</artifactId> Line 36: </dependency> I think we should also require javax.inject:javax.inject here. Line 37: </dependencies> Line 38: Line 39: <build> Line 40: <plugins> .................................................... File backend/manager/modules/scheduler/src/main/java/org/ovirt/engine/core/utils/timer/SchedulerUtilQuartzImpl.java Line 44 Line 45 Line 46 Line 47 Line 48 So the lock manager wasn't actually required here? -- To view, visit http://gerrit.ovirt.org/19887 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I126fa3f4240e81814ec9e902cb2e93ce364589ff Gerrit-PatchSet: 2 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Roy Golan <[email protected]> Gerrit-Reviewer: Juan Hernandez <[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
