Skip to content

REST call to get activities on an adjunct#836

Merged
asfgit merged 5 commits intoapache:masterfrom
ahgittin:adjunct-activities-rest-api
Oct 6, 2017
Merged

REST call to get activities on an adjunct#836
asfgit merged 5 commits intoapache:masterfrom
ahgittin:adjunct-activities-rest-api

Conversation

@ahgittin
Copy link
Copy Markdown
Contributor

@ahgittin ahgittin commented Sep 20, 2017

A single small commit to get via REST the activities related to an adjunct, and descendant tasks.

This depends on #821 and #835 -- where most of the prep work is done -- but once those are in the only changes should be those in d1ab00c .

@ahgittin ahgittin force-pushed the adjunct-activities-rest-api branch from 935c743 to d1ab00c Compare September 25, 2017 12:18
Conflict - tweaks to AdjunctResource, easily resolved
@ahgittin
Copy link
Copy Markdown
Contributor Author

ahgittin commented Oct 4, 2017

#821 now merged and #835 almost ready for review; this updated but still depends on #835

still the only commit beyond that needing review here is
d1ab00c

@aledsage
Copy link
Copy Markdown
Contributor

aledsage commented Oct 5, 2017

By the way, I don't entirely trust merge commits inside a pull request. It has seemed to me previously that /files in github has not shown me all the code changes because some are implicit in a merge commit. I'm not sure of the exact situation though. It happened when I was once trying to take a PR's changes into a maintenance branch, but it seemed that the .patch github mechanism didn't treat the merge commits as I'd have expected.

I know that you have quite a sophisticated way of using git branches and merges locally, but when it reaches the PR stage is it not possible to just have it rebased against master such that it appears as a series of normal commits on top of brooklyn-server master?

Copy link
Copy Markdown
Contributor

@aledsage aledsage left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes in d1ab00c LGTM.

@ApiParam(value = "Application ID or name", required = true) @PathParam("application") String applicationId,
@ApiParam(value = "Entity ID or name", required = true) @PathParam("entity") String entityId,
@ApiParam(value = "Adjunct ID or name", required = true) @PathParam("adjunct") String adjunctToken,
@ApiParam(value = "Max number of tasks, or -1 for all (default 200)", required = false)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a fan of -1 for all. But it's already used in EntityApi.listTasks so I'll live with it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes for me it is simply "least bad"

@ahgittin
Copy link
Copy Markdown
Contributor Author

ahgittin commented Oct 5, 2017

@aledsage the one thing i find with merges is that it's a bit difficult to see changes made to resolve conflicts as part of the merge commit. i always forget the exact command but manage to find it.

however it keeps much better history, e.g.

% git log --oneline --graph master..
*   cea2d34 Merge branch 'tasks-better-tree' into adjunct-activities-rest-api
|\  
| * 9213f0e fix message publish synching to guarantee in-order delivery
| * 508183b more assertion routines, map equality and unordered iterable equality
| * 8b72769 Tasks.tryQueueing won't queue if calling thread is interrupted
| * 9888870 switch queue-or-submit-blocking-then-get invocations to new simpler DynamicTasks.get. but note some things fail getImmediately if they are running a queueing context eg EffectorSayHiTest, until fixed in the next PR.
* |   b042989 Merge branch 'tasks-better-tree' into adjunct-activities-rest-api
|\ \  
| |/  
| * 0c2e1f6 Merge branch 'master' into tasks-4
| * bb26d32 change when cancellation is done for getImmediate - means effector invocation now works
| * d7b086b Merge branch 'tasks-better' into tasks-better-tree
* | d1ab00c REST API for entity adjunct activities
* | cf56492 Merge branch 'tasks-better-tree' into adj10
|/  
* 6dfe498 deprecated since is now 0.13.0 not 0.12.0
* 80446ab Merge branch 'master' into tasks-better-tree
* 130a29b fix tests that asserted specific tasks (as there are now more)
* 79cc9bc task visibility: ensure all tasks have a name, updating exec.submit() methods to take name
* aeecd3e task GC and visibility: tidy GC code, don't delete some things eg subscriptions quite so aggressively
* b0556de include adjunct info as a subscription description
* e1f948a task visibility: entity initialization
* 0a1acec fix deadlock in initial publication of sensors on setting up a subscription
* 84d24d1 fix visibility: entity init runs in same thread
* d4c9fe1 entity adjuncts have extra tag for execution context, used in subscription delivery
* 8ecf395 task visibility: better names for config retrieval tasks
* 4430f76 task optimization: some queued-or-submitted tasks use foreground for executing
* 7f4d7bd task visibility: entity mgmt create and startup wrapped in its own task

it's usually prettier but even here not hard to see d1ab00c as the important mainline commit.

more impressive is:

% git log --oneline --graph tasks-better-tree..
* cea2d34 Merge branch 'tasks-better-tree' into adjunct-activities-rest-api
* b042989 Merge branch 'tasks-better-tree' into adjunct-activities-rest-api
* d1ab00c REST API for entity adjunct activities
* cf56492 Merge branch 'tasks-better-tree' into adj10

making it super easy to see. and bear in mind this at one point built on 3 other branches. if i'd ever rebased this or any of those branches, the ability to reconcile changes would be gone. with git merge however it was always straightforward.

ahgittin added a commit to ahgittin/brooklyn-server that referenced this pull request Oct 6, 2017
@asfgit asfgit merged commit 92b9cc6 into apache:master Oct 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants