Shireesh Anjal has posted comments on this change.
Change subject: engine: Added infrastructure for providing a canDoactions in
case of locks
......................................................................
Patch Set 1: (9 inline comments)
While the logic seems to be correct, I think usage of Pair class to achieve
this is a bit not-so-object-oriented, and a little difficult to read.
I would have tried to leave the command base as is, and enhance the
InMemoryLockManager to "find" the appropriate error code, depending on the
locking group. I think the error message for not being able to acquire lock on
a particular (entity + locking group) can be retrieved in a generic manner, and
there is no need for each command class to provide this information.
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/CommandBase.java
Line 1471: protected LockManager getLockManager() {
Line 1472: return LockManagerFactory.getLockManager();
Line 1473: }
Line 1474:
Line 1475: protected Map<String, Pair<String, String>> getExclusiveLocks() {
It'll be useful to add a javadoc comment here explaining what exactly this
method is supposed to return. I'm asking because the Map of string to pair is
not very self-explanatory.
Line 1476: return null;
Line 1477: }
Line 1478:
Line 1479: protected Map<String, Pair<String, String>> getSharedLocks() {
Line 1475: protected Map<String, Pair<String, String>> getExclusiveLocks() {
Line 1476: return null;
Line 1477: }
Line 1478:
Line 1479: protected Map<String, Pair<String, String>> getSharedLocks() {
Here as well.
Line 1480: return null;
Line 1481: }
Line 1482:
Line 1483: @Override
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/lock/InMemoryLockManager.java
Line 40: @TransactionAttribute(TransactionAttributeType.NOT_SUPPORTED)
Line 41: @Local(LockManager.class)
Line 42: public class InMemoryLockManager implements LockManager,
LockManagerMonitorMXBean {
Line 43:
Line 44: private static final Pair<Boolean, Set<String>>
LOCK_INSER_SUCCESS_RESULT = new Pair<Boolean, Set<String>>(Boolean.TRUE,
Collections.<String>emptySet());
LOCK_INSER*T*_SUCCESS_RESULT ?
Line 45: /** A map which is contains all internal representation of locks
**/
Line 46: private final Map<String, InternalLockView> locks = new
HashMap<String, InternalLockView>();
Line 47: /** A lock which is used to synchronized acquireLock(),
acquireLockWait() and releaseLock() operations **/
Line 48: private final Lock globalLock = new ReentrantLock();
Line 142: }
Line 143:
Line 144: @Override
Line 145: public boolean releaseLock(String lockId) {
Line 146: log.warnFormat("The following lock is going to be released
via external call, lockId {0}, error message can be left for shared lock",
I don't understand what this additional part of the message means: "error
message can be left for shared lock". Can you please elaborate?
Line 147: lockId);
Line 148: globalLock.lock();
Line 149: try {
Line 150: InternalLockView lock = locks.get(lockId);
Line 303: }
Line 304: }
Line 305:
Line 306: /**
Line 307: * The following class is represents different locks which are
kept inside InMemoryLockManager
"is" can be removed from the comment
Line 308: */
Line 309: private class InternalLockView {
Line 310:
Line 311: /** Number for shared locks **/
Line 311: /** Number for shared locks **/
Line 312: private int count;
Line 313: /** Indicate if the lock is exclusive and not allowing any
other exclusive/shared locks with the same key **/
Line 314: private final boolean exclusive;
Line 315: /** Contains a error messages for that key **/
"a" can be removed from the comment
Line 316: private List<String> messages;
Line 317:
Line 318: public InternalLockView(int count, String message, boolean
exclusive) {
Line 319: this.count = count;
Line 312: private int count;
Line 313: /** Indicate if the lock is exclusive and not allowing any
other exclusive/shared locks with the same key **/
Line 314: private final boolean exclusive;
Line 315: /** Contains a error messages for that key **/
Line 316: private List<String> messages;
If getMessages() returns a Set, why not define this variable as Set<String>
itself, so that the conversion from list to set can be avoided inside
getMessages() ?
Line 317:
Line 318: public InternalLockView(int count, String message, boolean
exclusive) {
Line 319: this.count = count;
Line 320: this.exclusive = exclusive;
Line 317:
Line 318: public InternalLockView(int count, String message, boolean
exclusive) {
Line 319: this.count = count;
Line 320: this.exclusive = exclusive;
Line 321: messages = new ArrayList<String>();
This initialization can be done as part of variable declaration itself..
Line 322: messages.add(message);
Line 323: }
Line 324:
Line 325: public boolean getExclusive() {
....................................................
File
backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/lock/InMemoryLockManagerTest.java
Line 83: assertTrue(lockResult.getSecond().contains(ERROR1));
Line 84: assertEquals(lockResult.getSecond().size(), 1);
Line 85: lockResult =
LockManagerFactory.getLockManager().acquireLock(updateLock2);
Line 86: assertFalse(lockResult.getFirst());
Line 87: assertTrue(lockResult.getSecond().contains(ERROR2));
It will be good if you write a test case for shared lock failure resulting in
more than one error messages. It'll help me understand that part of the
functionality better.
Line 88: assertEquals(lockResult.getSecond().size(), 1);
Line 89:
LockManagerFactory.getLockManager().releaseLock(updateAndLockLock);
Line 90:
assertTrue(LockManagerFactory.getLockManager().acquireLock(lockLock1).getFirst());
Line 91:
assertTrue(LockManagerFactory.getLockManager().acquireLock(updateLock2).getFirst());
--
To view, visit http://gerrit.ovirt.org/11738
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I87698da36ddb6e7556e98612d299da68f2eb53dc
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Michael Kublin <[email protected]>
Gerrit-Reviewer: Allon Mureinik <[email protected]>
Gerrit-Reviewer: Arik Hadas <[email protected]>
Gerrit-Reviewer: Barak Azulay <[email protected]>
Gerrit-Reviewer: Daniel Erez <[email protected]>
Gerrit-Reviewer: Eli Mesika <[email protected]>
Gerrit-Reviewer: Michael Kublin <[email protected]>
Gerrit-Reviewer: Omer Frenkel <[email protected]>
Gerrit-Reviewer: Shireesh Anjal <[email protected]>
Gerrit-Reviewer: Yair Zaslavsky <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
