Michael Kublin has posted comments on this change. Change subject: core: introducing scheduling framework ......................................................................
Patch Set 1: (10 inline comments) Yet another concurrency and scheduling framework? I want clear explanation why we need this. Why I need a special future? Why I need to override a functionality of future? Why I need to write a special synchronization framework? Why java.util.concurrentis not enought ? Why any of it classes are not used? Why if this is not enough http://code.google.com/p/guava-libraries is not used? Why code is look like it is written in java 1.3? .................................................... 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: Why to pass it at all? ThreadPool is implement interface 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); Maybe because that classes are implements unblocking collections, which is build based upon compare and set algorithm and not required any synchronization? 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 63: public void run() { Line 64: long closestExpirationSec = Integer.MAX_VALUE; Line 65: while (true) { Line 66: if (closestExpirationSec > 0) { Line 67: try { Saggi, java.util.concurrent.locks.Lock is custom made mechanism? Please read java doc: http://docs.oracle.com/javase/6/docs/api/java/util/concurrent/locks/package-summary.html 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) { closestExpirationSec * 1000 - this is interface? http://docs.oracle.com/javase/6/docs/api/java/util/concurrent/TimeUnit.html Line 69: this.wait(closestExpirationSec * 1000); Line 70: } Line 71: } catch (InterruptedException e) { Line 72: // ignore Line 73: } Line 74: } Line 75: Line 76: closestExpirationSec = Integer.MAX_VALUE; Line 77: And what you are trying to do? From code you are trying to do busy-wait 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()) { .................................................... 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: I know what is Future, and I don't understand why you are trying to write future by yourself and it is not written correct 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: Saggi, you are wrong and you wrote code against convention Line 30: public boolean cancel(boolean mayInterruptIfRunning) { Line 31: synchronized (this) { Line 32: if (!_cancelled) { Line 33: _cancelled = _parent.cancel(mayInterruptIfRunning); Line 40: public V get() throws InterruptedException, ExecutionException { Line 41: _flag.await(); Line 42: return _result; Line 43: } Line 44: Really? TimeoutException - if the wait timed out -? How? 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 that we need this class 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) { Use google and read what is mean, if you are writing a concurrent code in java you should know basic principles of concurrency in java Line 52: assert (isDone() == false); Line 53: _result = result; Line 54: _flag.countDown(); Line 55: _done = true; -- 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
