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

Trigger management #101

Merged
merged 9 commits into from Nov 1, 2017

Conversation

Projects
None yet
4 participants
@abaruni
Contributor

abaruni commented Oct 23, 2017

this PR is for the fetching of the current configuration and status of a feed trigger

@abaruni

This comment has been minimized.

Show comment
Hide comment
@abaruni
Contributor

abaruni commented Oct 23, 2017

@jthomas

This comment has been minimized.

Show comment
Hide comment
@jthomas

jthomas Oct 24, 2017

Member

Can the READ response be extended to include the triggersLeft value?

Related to: apache/incubator-openwhisk#1398

Member

jthomas commented Oct 24, 2017

Can the READ response be extended to include the triggersLeft value?

Related to: apache/incubator-openwhisk#1398

@jasonpet

This comment has been minimized.

Show comment
Hide comment
@jasonpet

jasonpet Oct 24, 2017

Contributor

Not sure how valid issue #1398 is anymore. That was created before we set infinite triggers as the default for alarms. The READ simply reads from the database and the triggers left is not stored in the database. It is an in-memory count on the provider itself. We really do not want to be doing database writes on every trigger fire. We could try to read from a provider endpoint to get this value. However, this is not trivial now that we have multiple provider instances (as part of our redundancy and load balancing support). We would have to compute the endpoint url to use (by getting the worker id from the db first).

Contributor

jasonpet commented Oct 24, 2017

Not sure how valid issue #1398 is anymore. That was created before we set infinite triggers as the default for alarms. The READ simply reads from the database and the triggers left is not stored in the database. It is an in-memory count on the provider itself. We really do not want to be doing database writes on every trigger fire. We could try to read from a provider endpoint to get this value. However, this is not trivial now that we have multiple provider instances (as part of our redundancy and load balancing support). We would have to compute the endpoint url to use (by getting the worker id from the db first).

@jasonpet

This comment has been minimized.

Show comment
Hide comment
@jasonpet

jasonpet Oct 24, 2017

Contributor

@abaruni - the legacy alarms code created some trigger entries in the trigger database with a _ for the namespace. It was not until the creation of the __OW_NAMESPACE variable that we always started using the resolved namespace. In your current implementation the lookup for READ would always use the resolved namespace and would never work for users with older triggers that use _. Please see how we handle DELETE and do the same thing for READ.

Contributor

jasonpet commented Oct 24, 2017

@abaruni - the legacy alarms code created some trigger entries in the trigger database with a _ for the namespace. It was not until the creation of the __OW_NAMESPACE variable that we always started using the resolved namespace. In your current implementation the lookup for READ would always use the resolved namespace and would never work for users with older triggers that use _. Please see how we handle DELETE and do the same thing for READ.

@jthomas

This comment has been minimized.

Show comment
Hide comment
@jthomas

jthomas Oct 24, 2017

Member

@jasonpet Thanks for explaining, I see the reasoning that this requirement is less valid with the infinity max triggers value.

If the "triggers left" values are only stored in memory, does that mean in the event of a provider crash it's going to lose all that state?

Member

jthomas commented Oct 24, 2017

@jasonpet Thanks for explaining, I see the reasoning that this requirement is less valid with the infinity max triggers value.

If the "triggers left" values are only stored in memory, does that mean in the event of a provider crash it's going to lose all that state?

@jasonpet

This comment has been minimized.

Show comment
Hide comment
@jasonpet

jasonpet Oct 24, 2017

Contributor

@jthomas - yes, on a crash we would lose state and the count would start back from the beginning (initial value of maxTriggers). We are implementing a new alarms action that will fire until an end date (will stop once that date has been reached). This would be a more reliable option and give the user more control when creating a temporary alarms trigger.

Contributor

jasonpet commented Oct 24, 2017

@jthomas - yes, on a crash we would lose state and the count would start back from the beginning (initial value of maxTriggers). We are implementing a new alarms action that will fire until an end date (will stop once that date has been reached). This would be a more reliable option and give the user more control when creating a temporary alarms trigger.

@@ -170,7 +170,7 @@ module.exports = function(
var updatedTrigger = existing;
var status = {
'active': false,
'dateChanged': new Date().toISOString(),
'dateChanged': Date.now(),

This comment has been minimized.

@csantanapr

csantanapr Oct 25, 2017

Contributor

We want seconds not milliseconds , need to devide 1000 and round

@csantanapr

csantanapr Oct 25, 2017

Contributor

We want seconds not milliseconds , need to devide 1000 and round

@csantanapr

This comment has been minimized.

Show comment
Hide comment
@csantanapr

csantanapr Oct 25, 2017

Contributor

Need to handle how to deal when an old trigger has a different format for dateChange,

Contributor

csantanapr commented Oct 25, 2017

Need to handle how to deal when an old trigger has a different format for dateChange,

@jasonpet

This comment has been minimized.

Show comment
Hide comment
@jasonpet

jasonpet Oct 31, 2017

Contributor

Reviewed and tested most recent changes and they look good.

Contributor

jasonpet commented Oct 31, 2017

Reviewed and tested most recent changes and they look good.

@csantanapr csantanapr merged commit e34b0ea into apache:master Nov 1, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment