Allon Mureinik has posted comments on this change.
Change subject: engine: Introducing a queue for failovers event
......................................................................
Patch Set 3: (17 inline comments)
see inline
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/eventqueue/EventQueueMonitor.java
Line 31: @Singleton(name = "EventQueue")
Line 32: @ConcurrencyManagement(ConcurrencyManagementType.BEAN)
Line 33: @TransactionAttribute(TransactionAttributeType.SUPPORTS)
Line 34: @Local(EventQueue.class)
Line 35: public class EventQueueMonitor implements EventQueue {
general:
1. there's a lot of specific treatment for reconstruction here - and if we have
additional issues that would want the same treatment, it will have to be done
in a couple of places
2. some logging (e.g., when a task is submitted, started, finished) would be
nice.
Line 36:
Line 37: private static Log log =
LogFactory.getLog(EventQueueMonitor.class);
Line 38:
Line 39: private static final ConcurrentMap<Guid, ReentrantLock>
poolsLockMap = new ConcurrentHashMap<Guid, ReentrantLock>();
Line 33: @TransactionAttribute(TransactionAttributeType.SUPPORTS)
Line 34: @Local(EventQueue.class)
Line 35: public class EventQueueMonitor implements EventQueue {
Line 36:
Line 37: private static Log log =
LogFactory.getLog(EventQueueMonitor.class);
should be final
Line 38:
Line 39: private static final ConcurrentMap<Guid, ReentrantLock>
poolsLockMap = new ConcurrentHashMap<Guid, ReentrantLock>();
Line 40: private static final Map<Guid, Queue<Pair<Event,
FutureTask<EventResult>>>> poolsEventsMap =
Line 41: new HashMap<Guid, Queue<Pair<Event,
FutureTask<EventResult>>>>();
Line 70: Event currentEvent =
poolCurrentEventMap.get(storagePoolId);
Line 71: if (currentEvent != null) {
Line 72: switch (currentEvent.getEventType()) {
Line 73: case RECONSTRUCT:
Line 74: log.debugFormat("Current event was skiped because
of reconstruct is running now for pool {0}, event {1}",
s/skiped/skipped/
s/because of/because/
Line 75: storagePoolId, event);
Line 76: break;
Line 77: default:
Line 78: task = new FutureTask<EventResult>(callable);
Line 101: return queue;
Line 102: }
Line 103:
Line 104: private ReentrantLock getPoolLock(Guid poolId) {
Line 105: if (!poolsLockMap.containsKey(poolId)) {
the check and the put are not atomic - this is a possible race.
small, but a race.
Line 106: poolsLockMap.putIfAbsent(poolId, new ReentrantLock());
Line 107: }
Line 108: return poolsLockMap.get(poolId);
Line 109: }
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/eventqueue/Event.java
Line 5: public class Event {
Line 6:
Line 7: private EventType eventType;
Line 8: private Guid storagePoolId;
Line 9: private Guid domainId;
suggestion: perhaps storageDomainId would be clearer?
Line 10: private Guid vdsId;
Line 11:
Line 12: public Event(Guid storagePoolId, Guid domainId, Guid vdsId,
EventType eventType) {
Line 13: this.storagePoolId = storagePoolId;
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/eventqueue/EventQueue.java
Line 4:
Line 5: public interface EventQueue {
Line 6:
Line 7: /**
Line 8: * The following method should allow to submit asynchronous event
s/ asynchronous /an asynchronous /
Line 9: * Event will be submitted to queue and will be executed when after
that
Line 10: * @param event - description of event
Line 11: * @param callable - a code which should be run
Line 12: */
Line 5: public interface EventQueue {
Line 6:
Line 7: /**
Line 8: * The following method should allow to submit asynchronous event
Line 9: * Event will be submitted to queue and will be executed when after
that
s/Event/The event/
Line 10: * @param event - description of event
Line 11: * @param callable - a code which should be run
Line 12: */
Line 13: void submitEventAsync(Event event, Callable<EventResult> callable);
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/eventqueue/EventType.java
Line 5: DOMAINFAILOVER,
Line 6: DOMAINNOTOPERATIONAL,
Line 7: VDSSTOARGEPROBLEMS,
Line 8: DOMAINMONITORING,
Line 9: VDSCLEARCACHE;
please add "_" between words
....................................................
File
backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/ejb/BeanType.java
Line 10: SCHEDULER, // SchedulerUtil
Line 11: USERS_DOMAINS_CACHE,
Line 12: VDS_EVENT_LISTENER,
Line 13: LOCK_MANAGER,
Line 14: EVENTQUEUE_MANAGER;
I think EVENT_QUEUE_MANAGER will be more readbable
Line 15:
....................................................
File
backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/threadpool/ThreadPoolUtil.java
Line 86: public static <V> Future<V> execute(FutureTask<V> command) {
Line 87: try {
Line 88: return (Future<V>) es.submit(command);
Line 89: } catch (RejectedExecutionException e) {
Line 90: log.warn("The thread pool is out of limit. A submitted
task was rejected");
s/A submitted/The submitted/
s/task/event/
Also, is it possible to add some info here? perhaps the event's type or
something like that?
Line 91: throw e;
Line 92: }
Line 93: }
....................................................
File
backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/irsbroker/IIrsBroker.java
Line 1
this deletion is unrelated to the patch - please separate it
....................................................
File
backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/irsbroker/IrsBrokerCommand.java
Line 448:
Line 449: result = ResourceManager.getInstance()
Line 450: .getEventListener()
Line 451:
.masterDomainNotOperational(masterDomainId, storagePoolId);
Line 452: return result;
I'd return inline, but its a matter of taste, I guess
Line 453:
Line 454: }
Line 455: });
Line 456: throw new IRSNoMasterDomainException(exceptionMessage);
....................................................
Commit Message
Line 6:
Line 7: engine: Introducing a queue for failovers event
Line 8:
Line 9: The following patch will introduce a queue for failovers event.
Line 10: Idea is to use a follwoing queue inorder to ensure a correct order of
s/Idea/The idea/
s/a following queue/the following queue/
s/inorder/in order/
s/a correct/the correct/
Line 11: the following operations: reconstruct, domain recovery, domain
monitoring, etc...
Line 12: The appropriate event should be submitted to queue and will be
executed accorrding to
Line 13: internal logic of queue.
Line 14: Because of execution of events is done in separate thread no deadlock
should occurred, also
Line 8:
Line 9: The following patch will introduce a queue for failovers event.
Line 10: Idea is to use a follwoing queue inorder to ensure a correct order of
Line 11: the following operations: reconstruct, domain recovery, domain
monitoring, etc...
Line 12: The appropriate event should be submitted to queue and will be
executed accorrding to
s/queue/the queue/
Line 13: internal logic of queue.
Line 14: Because of execution of events is done in separate thread no deadlock
should occurred, also
Line 15: no need for excplicit additional locking
Line 16: These is a first patch it will introduce a queue and all stuff should
work as before, but
Line 10: Idea is to use a follwoing queue inorder to ensure a correct order of
Line 11: the following operations: reconstruct, domain recovery, domain
monitoring, etc...
Line 12: The appropriate event should be submitted to queue and will be
executed accorrding to
Line 13: internal logic of queue.
Line 14: Because of execution of events is done in separate thread no deadlock
should occurred, also
s/of execution/the execution/
s/in separate/in a separate/
s/occured/occur/
Line 15: no need for excplicit additional locking
Line 16: These is a first patch it will introduce a queue and all stuff should
work as before, but
Line 17: using of queue with small change will allow: to reject unneeded events
when some othe event is running,
Line 18: ensure order, ensure appropriate release of quartz threads, fix a races
Line 12: The appropriate event should be submitted to queue and will be
executed accorrding to
Line 13: internal logic of queue.
Line 14: Because of execution of events is done in separate thread no deadlock
should occurred, also
Line 15: no need for excplicit additional locking
Line 16: These is a first patch it will introduce a queue and all stuff should
work as before, but
s/These/This/
s/a first/the first./
Line 17: using of queue with small change will allow: to reject unneeded events
when some othe event is running,
Line 18: ensure order, ensure appropriate release of quartz threads, fix a races
Line 19: The following patch also introduce a using of queue, from different
places, including RecoveryStoragePoolCommand
Line 20:
Line 18: ensure order, ensure appropriate release of quartz threads, fix a races
Line 19: The following patch also introduce a using of queue, from different
places, including RecoveryStoragePoolCommand
Line 20:
Line 21: Change-Id: Ic1224feacdcdaaaf0b59d26105805ba7ef2a2fff
Line 22: Bug-Url: https://bugzilla.redhat.com/??????
please remove or add the PRD number
--
To view, visit http://gerrit.ovirt.org/9838
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic1224feacdcdaaaf0b59d26105805ba7ef2a2fff
Gerrit-PatchSet: 3
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Michael Kublin <[email protected]>
Gerrit-Reviewer: Allon Mureinik <[email protected]>
Gerrit-Reviewer: Barak Azulay <[email protected]>
Gerrit-Reviewer: Eli Mesika <[email protected]>
Gerrit-Reviewer: Liron Aravot <[email protected]>
Gerrit-Reviewer: Michael Kublin <[email protected]>
Gerrit-Reviewer: Moti Asayag <[email protected]>
Gerrit-Reviewer: Omer Frenkel <[email protected]>
Gerrit-Reviewer: Ravi Nori <[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
