Is iterative action possible?

[gunjanvora]

Is it possible to have multiple iteration of single action in an experiment? Say I want to run terminate-db-pod several times after each 600 sec.

{ "name": "all-our-microservices-should-be-healthy", "type": "probe", "tolerance": "true", "provider": { "type": "python", "module": "chaosk8s.probes", "func": "microservice_available_and_healthy", "arguments": { "name": "myapp", "ns": "myns" } } }, { "type": "action", "name": "terminate-db-pod", "provider": { "type": "python", "module": "chaosk8s.pod.actions", "func": "terminate_pods", "arguments": { "label_selector": "app=my-app", "name_pattern": "my-app-[0-9]$", "rand": true, "ns": "default" } }, "pauses": { "after": 5 } }

[toranb]

@dustinfarris I like the simplicity of this for sure. One blocker I see currently is that setProperties is called with a new object each time (w/ Object.assign) likely resulting in a notifyPropertyChange for each property in the component. I can't say this is the case for certain so I'll need you and I to do some home work here.

Here is what I would like to accompany this PR regarding test coverage that I couldn't seem to find myself searching around for a few minutes tonight.

1. Throw out my monkey patch for updateProps and instead look deeper at what is truly invoked at the ember metal layer when notifyPropertyChange is fired.
2. Use this to get a new snapshot/metric before the changes you've set out here
3. Confirm your changes (and setProperties) doesn't incur any additional work from ember (as I suspect it does looking at setProperties here)

@brettburley if you have any working knowledge of ember internals (ie: what gets notified /when) it would greatly improve our ability to confirm we aren't back in over-notify land. I want to make sure we go above and beyond if we plan to refactor this because @justinpark opened an issue related to this very problem in #37 recently.

[brettburley]

One blocker I see currently is that setProperties is called with a new object each time (w/ Object.assign) likely resulting in a notifyPropertyChange for each property in the component.

Fortunately, setProperties will noop when the new value is equal to the current value. So it's safe to call it with an entirely new hash. We will see the issue we discovered here https://github.com/toranb/ember-redux/pull/30#issuecomment-258061853 where unchanged objects may be re-set as a result of the original being mutated by Ember at runtime. But the notification frequency should match the behavior with the current check here: https://github.com/toranb/ember-redux/blob/master/addon/components/connect.js#L52

Regarding tests, I think we should definitely be counting number of re-renders. One way to do that is like I did here by tracking didRenders: https://github.com/toranb/ember-redux/pull/30, but there might be other ways to count the same. It would probably be nice to add a few tests that only change properties that aren't rendered and verifying that the component isn't re-rendered.

We could also add some tests that set up observers on the properties and ensure the observers don't fire when the property doesn't change.

[toranb]

@dustinfarris you mind implementing the re-render checks that @brettburley mentioned above? I was reading up on the mixin that does observer work but I think re-render maybe the most appropriate given this approach. The biggest challenge I have with re-render @brettburley is that if I have 2 properties on the component and only 1 changes - I assume a re-render is invoked but did we (or did we not) notifyPropertyChange for both props?

[toranb]

@dustinfarris could you push up the object.assign changes @brettburley mentioned? Without them we do indeed see a regression related to the notifyPropertyChange (using my new test in PR #42 )

here is the diff after using setProperties as you have it above

edit

I'm curious if the object.assign changes will solve this regression/ unlock the ability to merge this in the next few days (assuming everyone is happy with it as-is)

[brettburley]

I don't think Object.assign will change the behavior at all. Let's see after #42 is updated if the counts still mismatch... I would expect them to be the same in this case.

[toranb]

@dustinfarris could you rebase this now that PR #42 was merged? I found that your work w/ setProperties here is now regression free because ember already does the heavy lifting before it decides to notifyPropertyChange

[dustinfarris]

Ok, rebased and updated with @brettburley's suggestions—which simplified the code greatly as it is no longer necessary to transform the props object at all.

I've updated the description to match. This should be good to go.

[toranb]

@dustinfarris this is awesome! Just look at all the code we can delete! Thanks for all the hard work here

@brettburley this seems rock solid and ready to merge from my view. You like?

[brettburley]

This looks great! Thanks for all the work @dustinfarris.

[toranb]

@dustinfarris @brettburley this is now up on npm as part of the v1.9.0 release

[Lawouach]

Hi @gunjanvora,

This is not something currently designed in the toolkit. It reminds me of this request though. Might be worth chiming in this discussion if you can?