Yair Zaslavsky has posted comments on this change. Change subject: core: Inject TransactionSupport instead of JNDI lookups ......................................................................
Patch Set 3: (4 comments) Arent we to remove all the static methods and replace with non static ones? a. Will give us better testability b. I'm not the CDI wiz, but this is what I understand when I see getTransactionSupport, or injection of TransactionSupport, and I totally agree it should be injected. http://gerrit.ovirt.org/#/c/35637/3/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/gluster/GlusterSyncJob.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/gluster/GlusterSyncJob.java: Line 205: } Line 206: }); Line 207: } Line 208: Line 209: protected TransactionSupport getTransactionSupport() { Is this the only place besides Commands that has transactions ? Line 210: return Injector.get(TransactionSupport.class); Line 211: } Line 212: Line 213: /** http://gerrit.ovirt.org/#/c/35637/3/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/StorageDomainCommandBase.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/StorageDomainCommandBase.java: Line 460: protected IStorageHelper getStorageHelper(StorageDomain storageDomain) { Line 461: return StorageHelperDirector.getInstance().getItem(storageDomain.getStorageType()); Line 462: } Line 463: Line 464: protected void executeInNewTransaction(TransactionMethod<?> method) { Too bad the guy who added this method did not think of putting it in CommandBase so others could have enjoyed, though i don't think i'm such in favor of a one liner utility method. Care to move this to command base? Line 465: getTransactionSupport().executeInNewTx(method); Line 466: } Line 467: Line 468: private static final class LastTimeUsedAsMasterComp implements Comparator<StorageDomain>, Serializable { http://gerrit.ovirt.org/#/c/35637/3/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/storage/DeactivateStorageDomainCommandTest.java File backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/storage/DeactivateStorageDomainCommandTest.java: Line 95: public void setup() { Line 96: doReturn(dbFacade).when(cmd).getDbFacade(); Line 97: when(dbFacade.getStoragePoolDao()).thenReturn(storagePoolDAO); Line 98: when(dbFacade.getStorageDomainDao()).thenReturn(storageDomainDAO); Line 99: when(cmd.getTransactionSupport()).thenReturn(transactionSupport); > This code will repeat itself in every command test... Can we please have a +1 Line 100: } Line 101: Line 102: @Test Line 103: public void statusSetInMap() { http://gerrit.ovirt.org/#/c/35637/3/backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/transaction/TransactionSupport.java File backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/transaction/TransactionSupport.java: Line 210: /** Line 211: * Forces "REQUIRES_NEW" and executes given code in that scope Line 212: */ Line 213: Line 214: public static <T> T executeInNewTransaction(TransactionMethod<T> code) { shouldn't this one be eventually eliminate? this is a static version of the new non static one. please elaborate? Line 215: T result = null; Line 216: Transaction transaction = null; Line 217: Line 218: try { -- To view, visit http://gerrit.ovirt.org/35637 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia2a73887c76c85b17db6a3c2683d257aece10b59 Gerrit-PatchSet: 3 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Roy Golan <rgo...@redhat.com> Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com> Gerrit-Reviewer: Kanagaraj M <kmayi...@redhat.com> Gerrit-Reviewer: Maor Lipchuk <mlipc...@redhat.com> Gerrit-Reviewer: Sahina Bose <sab...@redhat.com> Gerrit-Reviewer: Yair Zaslavsky <yzasl...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches