Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

UI: Background Polling Step 1 - Foundational support & jobs #3893

Merged
merged 24 commits into from
Mar 4, 2018

Conversation

DingoEatingFuzz
Copy link
Contributor

First thing's first, gifs!

polling-jobs
Starting a couple jobs and watching allocations go through various states

polling-deployment
Deploying a new version of a service job

Details

This first step is the biggest step. It is all the underpinning required to leverage Nomad's blocking queries for long-polling updates for the UI. It's done using Ember and Ember Data, of course, as well as Ember Concurrency for task management and a fair amount of customizations to the adapter and serializer layers of Ember Data.

With that foundation in place, getting pages to watch resources is pretty straightforward. Future PRs for this background polling project should be much smaller and come at a regular pace.

xhr.abort();
}
}
},
Copy link
Contributor

Choose a reason for hiding this comment

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

👏 👏 👏 wow this file - super bummer there's not a better way to cancel requests than to manually track and reach down into the jquery XHR object, but still this is 👌

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah. Though with a promise-based workflow, there isn't a good place to expose the XHR. And promises aren't cancelable, so that avenue doesn't work either. Maybe we need AjaxPromiseProxies to come back from store methods.

@@ -66,14 +66,18 @@ export default Component.extend(WindowResizable, {
this.renderChart();
},

updateChart: observer('_data.@each.{value,label,className}', function() {
this.renderChart();
Copy link
Contributor

Choose a reason for hiding this comment

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

So with the more live updating - does this.renderChart() need to be an e-c task? Just curious if there's a small chance of race between updating the data more frequently, drawing the chart, and navigating away from the page/component.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The chart rendering is entirely synchronous, so it should be fine.

@@ -4,6 +4,8 @@ import attr from 'ember-data/attr';
import { fragmentOwner, fragmentArray } from 'ember-data-model-fragments/attributes';
import sumAggregation from '../utils/properties/sum-aggregation';

const maybe = arr => arr || [];
Copy link
Contributor

Choose a reason for hiding this comment

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

ha, nice

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Loosely inspired by Haskell.

@@ -0,0 +1,10 @@
import RSVP from 'rsvp';
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems weird that the fn is timeout which is also the default export from a file called wait. Not a big deal, but I'd rename the file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. I just save as'd the existing timeout.js. I'll rename the function here.

@@ -79,6 +79,11 @@ test('when switching namespaces, the app redirects to /jobs with the new namespa

visit(`/jobs/${job.id}?namespace=${namespace.name}`);

// andThen(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you mean to leave this in?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Lol, no.

.mapBy('id')
.sort(),
['node-1-2', 'node-2-1', 'node-2-2'],
'The deleted allocation is removed from the store and the allocations associated with the other node are untouched'
Copy link
Contributor

Choose a reason for hiding this comment

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

🎖️

run(() => {
store._push(payload);
store._didUpdateAll(modelName);
});
Copy link
Contributor

Choose a reason for hiding this comment

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

huh, TIL that does look handy

Copy link
Contributor

@meirish meirish left a comment

Choose a reason for hiding this comment

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

This looks really great and you did a nice job of separating and packaging up the complexity of the polling parts. I had one question about a race (maybe it's not an issue because d3 is fast enough™?), but other than that mega 👍 here.

Now that blocking queries are going to be in play, We can no longer
pretend the two requests are one, since they have independent nomad
indices.
- Service to manage X-Nomad-Index values
- Adapter method for reloading relationships with additional params
- Pattern for watching models and model relationships using EC
- Key data using datum label
  (when something goes from number, to zero, to number again)
- Retain active and inactive classes across data updates
Ember Data doesn't do this by default, instead opting to be as
non-destructive as possible. However, this is desired behavior.
When the findAll response from the server no longer has them.
@DingoEatingFuzz DingoEatingFuzz merged commit 2f93dea into f-ui-polling Mar 4, 2018
@DingoEatingFuzz DingoEatingFuzz deleted the f-ui-polling-foundation branch March 4, 2018 02:17
@DingoEatingFuzz DingoEatingFuzz mentioned this pull request Mar 5, 2018
3 tasks
@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants