Ravi Nori has posted comments on this change.
Change subject: engine: Introducing a queue for failovers event
......................................................................
Patch Set 1: (10 inline comments)
My comments are mostly around code organization and method names
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/eventqueue/EventQueueMonitor.java
Line 109: }
Line 110: return poolsLockMap.get(poolId);
Line 111: }
Line 112:
Line 113: private static class InternalEventQueuRunnable implements
Runnable {
Please change name to InternalEventQueueThread
Line 114:
Line 115: private Guid storagePoolId;
Line 116: private ReentrantLock lock;
Line 117: private Map<Guid, Event> poolCurrentEventMap;
Line 142: }
Line 143: } finally {
Line 144: lock.unlock();
Line 145: }
Line 146: if (pair != null) {
Can we move everything below this to a new method with a meaningful name.
Getting a bit too long to read
Line 147: Future<EventResult> futureResult =
ThreadPoolUtil.execute(pair.getSecond());
Line 148: EventResult result;
Line 149: try {
Line 150: result = futureResult.get();
Line 151: if (result == null) {
Line 152: result = pair.getSecond().get();
Line 153: if (result != null &&
result.getEventType() == EventType.RECONSTRUCT) {
Line 154: log.infoFormat("Finished reconstruct
for pool {0}. Clearing all event queue",
Line 155: storagePoolId);
Please move everything below this to a new method with a meaningful name
Line 156: lock.lock();
Line 157: try {
Line 158: for (Pair<Event,
FutureTask<EventResult>> task : poolsEventsMap.get(storagePoolId)) {
Line 159: log.infoFormat("The following
task {0} was cancelled, because of recosntruct was run before",
....................................................
File
backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/irsbroker/IrsBrokerCommand.java
Line 442: final Guid masterDomainId,
Line 443: final String exceptionMessage,
Line 444: final String logMessage) {
Line 445:
Line 446: ((EventQueue)
EjbUtils.findBean(BeanType.EVENTQUEUE_MANAGER,
BeanProxyType.LOCAL)).submitEventSync(new Event(_storagePoolId,
Very hard to read, can you please modularize.
Line 447: masterDomainId,
Line 448: EventType.RECONSTRUCT),
Line 449: new Callable<EventResult>() {
Line 450: @Override
Line 1083: updateDomainInProblem(vdsId, vdsName, domainsInProblems);
Line 1084: }
Line 1085:
Line 1086: private void updateDomainInProblem(final Guid vdsId, final
String vdsName, final Set<Guid> domainsInProblems) {
Line 1087: if (domainsInProblems != null) {
Same as above, a bit hard to read
Line 1088: ((EventQueue)
EjbUtils.findBean(BeanType.EVENTQUEUE_MANAGER,
BeanProxyType.LOCAL)).submitEventSync(new Event(_storagePoolId,
Line 1089: null,
Line 1090: EventType.DOMAINMONITORING),
Line 1091: new Callable<EventResult>() {
....................................................
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
please fix typo follwoing to following
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
Typo accorrding->according
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 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 15: no need for excplicit additional locking
excplicit -> explicit
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 19:
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
These -> These
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:
Line 20: Change-Id: Ic1224feacdcdaaaf0b59d26105805ba7ef2a2fff
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 17: using of queue with small change will allow: to reject unneeded events
when some othe event is running,
events when some othe event -> events when some other event
Line 18: ensure order, ensure appropriate release of quartz threads, fix a races
Line 19:
Line 20: Change-Id: Ic1224feacdcdaaaf0b59d26105805ba7ef2a2fff
Line 21: Bug-Url: https://bugzilla.redhat.com/??????
--
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: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Michael Kublin <[email protected]>
Gerrit-Reviewer: Barak Azulay <[email protected]>
Gerrit-Reviewer: Eli Mesika <[email protected]>
Gerrit-Reviewer: Michael Kublin <[email protected]>
Gerrit-Reviewer: Ravi Nori <[email protected]>
Gerrit-Reviewer: Yair Zaslavsky <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
