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

Reply via email to