Saggi Mizrahi has posted comments on this change.
Change subject: core: introducing scheduling framework
......................................................................
Patch Set 1: (17 inline comments)
....................................................
File
backend/manager/modules/taskmgr/src/main/java/org/ovirt/engine/core/taskmgr/scheduler/SimpleOperationScheduler.java
Line 36: }
Line 37: }
Line 38: final private ArrayList<OperationEntry<?>> _tasks;
Line 39: final private Executor _executor;
Line 40:
No need to use a concrete class when an interface is sufficient.
Line 41: public SimpleOperationScheduler(Executor executor) {
Line 42: _tasks = new ArrayList<OperationEntry<?>>();
Line 43: _executor = executor;
Line 44: }
Line 36: }
Line 37: }
Line 38: final private ArrayList<OperationEntry<?>> _tasks;
Line 39: final private Executor _executor;
Line 40:
Don't change see above.
Line 41: public SimpleOperationScheduler(Executor executor) {
Line 42: _tasks = new ArrayList<OperationEntry<?>>();
Line 43: _executor = executor;
Line 44: }
Line 47: int timeoutSec) {
Line 48: task.registerWaitObject(this);
Line 49: synchronized (_tasks) {
Line 50: Calendar timeout = Calendar.getInstance();
Line 51: timeout.add(Calendar.SECOND, timeoutSec);
I'm not handing a queue, why should I use one.
Line 52: _tasks.add(new OperationEntry<T>(task, callback, timeout));
Line 53: }
Line 54: /* Do a run in case the task finished before we registered
Line 55: * for notifications.
Line 52: _tasks.add(new OperationEntry<T>(task, callback, timeout));
Line 53: }
Line 54: /* Do a run in case the task finished before we registered
Line 55: * for notifications.
Line 56: */
You can't notify unless you are in a synchronized{} scope.
In any case, this is going away in favor of a more generic notification
interface
Line 57: synchronized (this) {
Line 58: this.notify();
Line 59: }
Line 60:
Line 63: public void run() {
Line 64: long closestExpirationSec = Integer.MAX_VALUE;
Line 65: while (true) {
Line 66: if (closestExpirationSec > 0) {
Line 67: try {
Because we are using the built in mechanisms
Line 68: synchronized (this) {
Line 69: this.wait(closestExpirationSec * 1000);
Line 70: }
Line 71: } catch (InterruptedException e) {
Line 64: long closestExpirationSec = Integer.MAX_VALUE;
Line 65: while (true) {
Line 66: if (closestExpirationSec > 0) {
Line 67: try {
Line 68: synchronized (this) {
This is the interface.
Line 69: this.wait(closestExpirationSec * 1000);
Line 70: }
Line 71: } catch (InterruptedException e) {
Line 72: // ignore
Line 71: } catch (InterruptedException e) {
Line 72: // ignore
Line 73: }
Line 74: }
Line 75:
I'm looking for a minimum and I have to start somewhere
Line 76: closestExpirationSec = Integer.MAX_VALUE;
Line 77:
Line 78: synchronized (_tasks) {
Line 79: for (int i = 0; i < _tasks.size(); i++) {
Line 73: }
Line 74: }
Line 75:
Line 76: closestExpirationSec = Integer.MAX_VALUE;
Line 77:
But this is not what I'm trying to do.
Line 78: synchronized (_tasks) {
Line 79: for (int i = 0; i < _tasks.size(); i++) {
Line 80: final OperationEntry<?> task = _tasks.get(i);
Line 81: if (task.isExpired() || task.isDone()) {
Line 76: closestExpirationSec = Integer.MAX_VALUE;
Line 77:
Line 78: synchronized (_tasks) {
Line 79: for (int i = 0; i < _tasks.size(); i++) {
Line 80: final OperationEntry<?> task = _tasks.get(i);
You don't run the task, you invoke the callback.
Line 81: if (task.isExpired() || task.isDone()) {
Line 82: _tasks.remove(i);
Line 83: i--;
Line 84: _executor.execute(new Runnable() {
Line 77:
Line 78: synchronized (_tasks) {
Line 79: for (int i = 0; i < _tasks.size(); i++) {
Line 80: final OperationEntry<?> task = _tasks.get(i);
Line 81: if (task.isExpired() || task.isDone()) {
The proper way is to use a linked list. But it's test code that had many
iterations.
Line 82: _tasks.remove(i);
Line 83: i--;
Line 84: _executor.execute(new Runnable() {
Line 85: public void run() {
....................................................
File
backend/manager/modules/taskmgr/src/main/java/org/ovirt/engine/core/taskmgr/scheduler/WaitedFutureMixin.java
Line 3: import java.util.concurrent.CountDownLatch;
Line 4: import java.util.concurrent.ExecutionException;
Line 5: import java.util.concurrent.TimeUnit;
Line 6: import java.util.concurrent.TimeoutException;
Line 7:
Future is not a class it's an interface.
There is FutureTask but we do completely different things.
FutureTask implements a wrapper around a callable and we just add the logic to
handle the signalling.
You could create a WaitedFutureTask that does both using the mixin.
Line 8: public final class WaitedFutureMixin<V> implements WaitedFuture<V> {
Line 9:
Line 10: public interface Parent {
Line 11:
Line 25: _result = null;
Line 26: _waitObj = null;
Line 27: _parent = parent;
Line 28: }
Line 29:
No need, we are implementing an interface.
Line 30: public boolean cancel(boolean mayInterruptIfRunning) {
Line 31: synchronized (this) {
Line 32: if (!_cancelled) {
Line 33: _cancelled = _parent.cancel(mayInterruptIfRunning);
Line 35:
Line 36: return _cancelled;
Line 37: }
Line 38: }
Line 39:
Interfaces
Line 40: public V get() throws InterruptedException, ExecutionException {
Line 41: _flag.await();
Line 42: return _result;
Line 43: }
Line 40: public V get() throws InterruptedException, ExecutionException {
Line 41: _flag.await();
Line 42: return _result;
Line 43: }
Line 44:
This is exactly what we do
Line 45: public V get(long timeout, TimeUnit unit) throws
InterruptedException,
Line 46: ExecutionException, TimeoutException {
Line 47: _flag.await(timeout, unit);
Line 48: return _result;
Line 46: ExecutionException, TimeoutException {
Line 47: _flag.await(timeout, unit);
Line 48: return _result;
Line 49: }
Line 50:
I don't think you understand what this class is for.
Line 51: public void setResult(V result) {
Line 52: assert (isDone() == false);
Line 53: _result = result;
Line 54: _flag.countDown();
Line 47: _flag.await(timeout, unit);
Line 48: return _result;
Line 49: }
Line 50:
Line 51: public void setResult(V result) {
I don't follow
Line 52: assert (isDone() == false);
Line 53: _result = result;
Line 54: _flag.countDown();
Line 55: _done = true;
Line 54: _flag.countDown();
Line 55: _done = true;
Line 56: synchronized (this) {
Line 57: if (_waitObj != null) {
Line 58: synchronized (_waitObj) {
We are replacing this. This is a WIP
Line 59: _waitObj.notify();
Line 60: }
Line 61: }
Line 62: }
--
To view, visit http://gerrit.ovirt.org/13795
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I07253e8465776482bbdade21d78e9b40eeaadbb5
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Yair Zaslavsky <[email protected]>
Gerrit-Reviewer: Michael Kublin <[email protected]>
Gerrit-Reviewer: Ravi Nori <[email protected]>
Gerrit-Reviewer: Saggi Mizrahi <[email protected]>
Gerrit-Reviewer: Yair Zaslavsky <[email protected]>
Gerrit-Reviewer: Yaniv Bronhaim <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
