brooklyn-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From aledsage <...@git.apache.org>
Subject [GitHub] incubator-brooklyn pull request: CLI commands for manipulating cat...
Date Mon, 18 May 2015 21:16:19 GMT
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/617#discussion_r30548244
  
    --- Diff: core/src/main/java/brooklyn/entity/rebind/PeriodicDeltaChangeListener.java ---
    @@ -247,68 +221,73 @@ void stop() {
             stop(Duration.TEN_SECONDS, Duration.ONE_SECOND);
         }
         void stop(Duration timeout, Duration graceTimeoutForSubsequentOperations) {
    -        stopped = true;
    -        running = false;
    -        
    -        if (scheduledTask != null) {
    -            CountdownTimer expiry = timeout.countdownTimer();
    -            scheduledTask.cancel(false);
    +        synchronized (startStopMutex) {
    +            running = false;
                 try {
    -                waitForPendingComplete(expiry.getDurationRemaining().lowerBound(Duration.ZERO).add(graceTimeoutForSubsequentOperations));
    -            } catch (Exception e) {
    -                throw Exceptions.propagate(e);
    -            }
    -            scheduledTask.blockUntilEnded(expiry.getDurationRemaining().lowerBound(Duration.ZERO).add(graceTimeoutForSubsequentOperations));
    -            scheduledTask.cancel(true);
    -            boolean reallyEnded = Tasks.blockUntilInternalTasksEnded(scheduledTask, expiry.getDurationRemaining().lowerBound(Duration.ZERO).add(graceTimeoutForSubsequentOperations));
    -            if (!reallyEnded) {
    -                LOG.warn("Persistence tasks took too long to complete when stopping persistence
(ignoring): "+scheduledTask);
    -            }
    -            scheduledTask = null;
    -        }
    +                stopping = true;
    +
    +                if (scheduledTask != null) {
    +                    CountdownTimer expiry = timeout.countdownTimer();
    +                    try {
    +                        scheduledTask.cancel(false);  
    +                        waitForPendingComplete(expiry.getDurationRemaining().lowerBound(Duration.ZERO).add(graceTimeoutForSubsequentOperations));
    +                    } catch (Exception e) {
    +                        throw Exceptions.propagate(e);
    +                    }
    +                    scheduledTask.blockUntilEnded(expiry.getDurationRemaining().lowerBound(Duration.ZERO).add(graceTimeoutForSubsequentOperations));
    +                    scheduledTask.cancel(true);
    +                    boolean reallyEnded = Tasks.blockUntilInternalTasksEnded(scheduledTask,
expiry.getDurationRemaining().lowerBound(Duration.ZERO).add(graceTimeoutForSubsequentOperations));
    +                    if (!reallyEnded) {
    +                        LOG.warn("Persistence tasks took too long to terminate, when
stopping persistence, although pending changes were persisted (ignoring): "+scheduledTask);
    +                    }
    +                    scheduledTask = null;
    +                }
     
     
    -        // Discard all state that was waiting to be persisted
    -        synchronized (this) {
    -            deltaCollector = new DeltaCollector();
    +                // Discard all state that was waiting to be persisted
    +                synchronized (this) {
    +                    deltaCollector = new DeltaCollector();
    +                }
    +            } finally {
    +                stopCompleted = true;
    +                stopping = false;
    +            }
             }
         }
         
         /**
    -     * This method must only be used for testing. If required in production, then revisit
implementation!
          * @deprecated since 0.7.0, use {@link #waitForPendingComplete(Duration)}
          */
         @VisibleForTesting
         public void waitForPendingComplete(long timeout, TimeUnit unit) throws InterruptedException,
TimeoutException {
             waitForPendingComplete(Duration.of(timeout, unit));
         }
    +    /** Waits for any in-progress writes to be completed then for or any unwritten data
to be written. */
         @VisibleForTesting
         public void waitForPendingComplete(Duration timeout) throws InterruptedException,
TimeoutException {
    -        // Every time we finish writing, we increment a counter. We note the current
val, and then
    -        // wait until we can guarantee that a complete additional write has been done.
Not sufficient
    -        // to wait for `writeCount > origWriteCount` because we might have read the
value when almost 
    -        // finished a write.
    +        if (!isActive() && !stopping) return;
             
    -        long startTime = System.currentTimeMillis();
    -        long maxEndtime = timeout.isPositive() ? startTime + timeout.toMillisecondsRoundingUp()
: Long.MAX_VALUE;
    -        long origWriteCount = writeCount.get();
    -        while (true) {
    -            if (!isActive()) {
    -                return; // no pending activity;
    -            } else if (writeCount.get() > (origWriteCount+1)) {
    -                return;
    -            }
    -            
    -            if (System.currentTimeMillis() > maxEndtime) {
    -                throw new TimeoutException("Timeout waiting for pending complete of rebind-periodic-delta,
after "+Time.makeTimeStringRounded(timeout));
    +        CountdownTimer timer = timeout.isPositive() ? CountdownTimer.newInstanceStarted(timeout)
: CountdownTimer.newInstancePaused(Duration.PRACTICALLY_FOREVER);
    +        // wait for mutex, so we aren't tricked by an in-progress who has already recycled
the collector
    +        if (persistingMutex.tryAcquire(timer.getDurationRemaining().toMilliseconds(),
TimeUnit.MILLISECONDS)) {
    +            try {
    +                // now no one else is writing
    +                if (!deltaCollector.isEmpty()) {
    +                    // but there is data that needs to be written
    +                    persistNowSafely(true);
    --- End diff --
    
    I can see this is simpler than the previously `writeCount`. However, it means that test
code (calling `waitForPendingComplete`) will change the behaviour of the persistence. It no
longer just waits, but actually does the persist. Therefore it means a class of bug will no
longer be detected where the persisting was not happening for whatever reason.
    
    That's probably not an issue (because such an errors are unlikely, thought they could
arise from some deadlock scenarios for example).
    
    No particularly strong feelings.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

Mime
View raw message