Tasks subsystem improvements - adjuncts, names, efficiency, GC by ahgittin · Pull Request #835 · apache/brooklyn-server

ahgittin · 2017-09-20T16:02:30Z

Adjuncts' tasks run in their context with tag so can be found more easily
Better naming of tasks across the board (so the look nicer in the UI)
More tidies to same-thread execution and run more things in same thread
Tidied task GC code, and changed to GC less aggressively
Fix race in initialization tasks

This increases the number of tasks kept around because sensor events are actually interesting (previously they were always garbage collected). However this may impact performance at scale: that deserves some investigation. There may be a balance to be had re which tasks to keep around and for how long.

NB this builds on #816, review that first

…executing

…ption delivery

…iption

description could be used for more things, exposed more broadly, but this is needed for now to make sense of those tasks

…scriptions quite so aggressively

… methods to take name

…vocation now works

…ynamicTasks.get. but note some things fail getImmediately if they are running a queueing context eg EffectorSayHiTest, until fixed in the next PR.

means more things work in immediate mode. could be un-done if DynamicSequentialTask removes the DST manager thread. fixes a few failing tests in the previous commit.

both for initial subscriptions and in-life - two changes, synching in AttributesInternal for the in-life delivery, and in subscribe/publish for the initial

ahgittin · 2017-10-04T15:05:08Z

Completed this round of improvements. A few more improvements:

many dynamic tasks run in same thread where possible, reducing overhead and supporting immediate execution
fix bug where subscriptions could be delivered out of order, where last subscription might not correspond to last value

The last item, in 9213f0e, does a bit more synching so is slightly risky, but I've minimized this and I think it is the correct thing to do, consistent at least with other code paths. Testing so far is good with it and there are ample notes on why.

What I have not done here is deprecation or warning in getImmediately(Task), as suggested in #816. That's turning in to a big job so I'll do it in another PR from which I'll reference this (so you should see a message about it below shortly.)

aledsage

LGTM; only minor comments.

aledsage · 2017-10-05T11:29:14Z

+        // won't be in this block. an issue with _subscribe-and-get-initial_
+        // should be resolved by initial subscription queueing the publication
+        // to a context where locks are not held.
+        synchronized (values) {


TL;DR: I agree this is fine. Below are some notes from digging around in the synchronization code.

Looking at the other synchronization blocks, emitInternal will synchronize on EntityManagementSupport instance (when it calls getSubscriptionContext(). It will then call publish which will synchronize on LocalSubscriptionManager instance (in getSubscriptionsForEntitySensor).

However, I think we can trust both the EntityManagementSupport and the LocalSubscriptionManager to not call out to alien code while holding a lock on itself. Therefore we should be safe in that respect.

The code in AbstractEntity and AbstractGroupImpl looks a bit scary, where it first gets the lock on this values object and then on either abstractGroup.members or abstractEntity.children (but it's kind-of understandable why it does that and how that pattern avoids it; it would just be nicer if instead we didn't call out to alient code while holding the lock on values - but that's a bigger discussion than for this PR):

synchronized (getAttributesSynchObjectInternal()) { synchronized (members) {

aledsage · 2017-10-05T11:30:56Z

+    @Deprecated
    <T> Task<T> submit(Callable<T> callable);

+    /** {@link ExecutionManager#submit(String Runnable) */


Missing comma in String Runnable.

aledsage · 2017-10-05T11:33:05Z

-        return validateAll();
+        ExecutionContext exec = 
+            getBrooklynObject() instanceof EntityInternal ? ((EntityInternal)getBrooklynObject()).getExecutionContext() :
+            // getBrooklynObject() instanceof AbstractEntityAdjunct ? ((AbstractEntityAdjunct)getBrooklynObject()).getExecutionContext() :


Strong believer in comments next to commented out code to say when someone would uncomment it!

yep, in this case it should have been uncommented as part of this PR

aledsage · 2017-10-05T12:01:58Z

-        return subscriptionContext;
+        // see also AbstractManagementContext.getSubscriptionContext - needed for callbacks, to ensure the correct entity context is set
+        Map<String, ?> subscriptionFlags = ImmutableMap.of("tags", ImmutableList.of(BrooklynTaskTags.tagForContextEntity(entity)));
+        return new BasicSubscriptionContext(subscriptionFlags, qsm, entity);


Why instantiate this in the getSubscriptionContext rather than the constructor?
Do you really want a new instance every time it is called, rather than lazy-instantiation and returning the same one?

For the record, I don't think it should really matter (i.e. fine as-is) - at some point we should be able to delete lots of this code. It was originally written when folk could call entity constructors directly and then call methods on it (so the entity would not have a usable management context).

it's a thin shim so doesn't matter if we cache or recreate

would be nice if we could remove the whole non-deployment mgmt context stage. suspect there are things like initializers and maybe rebind steps that we do want to run before other things or be queued to be the first things run when management context is attached so it may be that this complexity is still necessary.

aledsage · 2017-10-05T12:04:30Z

-            } catch (Exception e) {
-                throw Exceptions.propagate(e);
-            }
+            return ec.get(Tasks.<List<Application>>builder().displayName("rebind").dynamic(false).body(() -> rebindImpl(classLoader, exceptionHandler, mode)).build());


Minor: I prefer splitting it onto multiple lines (this line is now 168 chars long, the first 12 being the indent).

definitely, line getting too long to see what it really does

aledsage · 2017-10-05T12:09:42Z

        if (destroyed.get()) throw new IllegalStateException("Cannot set entity on a destroyed entity adjunct");
        this.entity = entity;
-        this.execution = ((EntityInternal) entity).getExecutionContext();
+        this.execution = new BasicExecutionContext( getManagementContext().getExecutionManager(),


I'd prefer us to add managementContext.getExecutionContext(entity, adjunct) (so it follows the same pattern as for getSubscriptionContext).

yes, that's cleaner

aledsage · 2017-10-05T12:17:00Z

                .description("Details of the original task have been forgotten.")
                .body(Callables.returning((T)null)).build();
            ((BasicTask<T>)t).ignoreIfNotRun();
+            ((BasicTask<T>)t).cancelled = true;


Worth a comment saying why you're setting cancelled = true. I presume it's because we don't really want anyone executing the "gone" task, and more importantly that if we are GC'ing tasks then marking the "gone" task as cancelled will help with cleanup of sub-tasks that have lost their submitted-by-task reference?

never occurred to me that marking cancelled might help with GC. have added comments.

the only thing i was optimizing was the surprising expense of soft references (noted 30 lines earlier)

aledsage · 2017-10-05T12:21:58Z

-                EntityAsserts.assertAttributeEqualsEventually(entity, TestEntity.NAME, after);
-            }
-        });
+        Task<?> assertValue = entity.getExecutionContext().submit("assert attr equals", () -> EntityAsserts.assertAttributeEqualsEventually(entity, TestEntity.NAME, after));


Preference breaking a long line across multiple lines.

aledsage · 2017-10-05T12:28:11Z

+        // delivery should be in subscription order, so 123 then 456
+        entity.subscriptions().subscribe(ImmutableMap.of("notifyOfInitialValue", true), entity, TestEntity.SEQUENCE, listener);
+        // wait for the above delivery - otherwise it might get dropped
+        Asserts.succeedsEventually(MutableMap.of("timeout", Duration.seconds(5)), () -> { 


Don't bother setting timeouts - just rely on the defaults. If you want things to run faster locally, then set the Java system property -Dbrooklyn.test.defaultTimeout=5s.

ooh handy forgot about this!

aledsage · 2017-10-05T12:30:58Z

+            new Thread(set).start();
+        }
+
+        Asserts.succeedsEventually(MutableMap.of("timeout", Duration.seconds(5)), () -> { 


As above, don't bother with bespoke timeouts. (Same below).

ahgittin · 2017-10-05T16:03:54Z

thanks @aledsage - great comments, will adress then merge this and #836

ahgittin · 2017-10-06T00:05:09Z

good comments, and straightforward, all addressed

ahgittin added 14 commits September 18, 2017 17:10

task visibility: validation of config

37b6b11

task visibility: entity mgmt create and startup wrapped in its own task

7f4d7bd

task optimization: some queued-or-submitted tasks use foreground for …

4430f76

…executing

task visibility: better names for config retrieval tasks

8ecf395

entity adjuncts have extra tag for execution context, used in subscri…

d4c9fe1

…ption delivery

fix visibility: entity init runs in same thread

84d24d1

fix deadlock in initial publication of sensors on setting up a subscr…

0a1acec

…iption

task visibility: entity initialization

e1f948a

include adjunct info as a subscription description

b0556de

description could be used for more things, exposed more broadly, but this is needed for now to make sense of those tasks

task GC and visibility: tidy GC code, don't delete some things eg sub…

aeecd3e

…scriptions quite so aggressively

task visibility: ensure all tasks have a name, updating exec.submit()…

79cc9bc

… methods to take name

fix tests that asserted specific tasks (as there are now more)

130a29b

Merge branch 'master' into tasks-better-tree

80446ab

deprecated since is now 0.13.0 not 0.12.0

6dfe498

ahgittin mentioned this pull request Sep 25, 2017

REST call to get activities on an adjunct #836

Merged

ahgittin added 2 commits September 28, 2017 17:28

Merge branch 'tasks-better' into tasks-better-tree

d7b086b

change when cancellation is done for getImmediate - means effector in…

bb26d32

…vocation now works

ahgittin mentioned this pull request Sep 28, 2017

Tasks code improvements - prep for better presentation of activities #816

Merged

ahgittin added 5 commits October 4, 2017 03:00

Merge branch 'master' into tasks-4

0c2e1f6

switch queue-or-submit-blocking-then-get invocations to new simpler D…

9888870

…ynamicTasks.get. but note some things fail getImmediately if they are running a queueing context eg EffectorSayHiTest, until fixed in the next PR.

Tasks.tryQueueing won't queue if calling thread is interrupted

8b72769

means more things work in immediate mode. could be un-done if DynamicSequentialTask removes the DST manager thread. fixes a few failing tests in the previous commit.

more assertion routines, map equality and unordered iterable equality

508183b

fix message publish synching to guarantee in-order delivery

9213f0e

both for initial subscriptions and in-life - two changes, synching in AttributesInternal for the in-life delivery, and in subscribe/publish for the initial

ahgittin force-pushed the tasks-better-tree branch from d5b326a to 9213f0e Compare October 4, 2017 14:56

ahgittin mentioned this pull request Oct 4, 2017

[WIP] deprecate Task as values in config and getImmediately(Task) in favour of TaskFactory #853

Open

aledsage reviewed Oct 5, 2017

View reviewed changes

address review comments

2dcb0a0

ahgittin force-pushed the tasks-better-tree branch from 9b8d13e to 2dcb0a0 Compare October 6, 2017 00:12

asfgit merged commit 2dcb0a0 into apache:master Oct 6, 2017

asfgit pushed a commit that referenced this pull request Oct 6, 2017

This closes #835

a73ee17

Conversation

ahgittin commented Sep 20, 2017

Uh oh!

ahgittin commented Oct 4, 2017

Uh oh!

aledsage left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahgittin commented Oct 5, 2017

Uh oh!

ahgittin commented Oct 6, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants