Allow for activation store to accept user and request information #3798
Conversation
dubee
force-pushed
the
activation-store-user-request
branch
from
4e8a766
to
0140629
Compare
dubee
added
the
review
|
PG4 1863 🔵 |
Codecov Report
@@ Coverage Diff @@
## master #3798 +/- ##
========================================
- Coverage 85.29% 78.7% -6.6%
========================================
Files 146 147 +1
Lines 7057 7418 +361
Branches 413 464 +51
========================================
- Hits 6019 5838 -181
- Misses 1038 1580 +542
Continue to review full report at Codecov.
|
| @@ -48,7 +49,8 @@ trait ActivationStore { | |||
| * @param transid transaction ID for request | |||
| * @return Future containing the retrieved WhiskActivation | |||
| */ | |||
| def get(activationId: ActivationId)(implicit transid: TransactionId): Future[WhiskActivation] | |||
| def get(activationId: ActivationId, user: Option[Identity] = None, request: Option[HttpRequest] = None)( | |||
There was a problem hiding this comment.
Why make user and request an Option? When would you not pass these values?
There was a problem hiding this comment.
@markusthoemmes, since there exists situations were the user and request information may not be know, DB polling for blocking invocations, do you think we should leave those parameters as optional?
There was a problem hiding this comment.
@markusthoemmes, certain activation stores may need to maintain a separate DB to track activations for blocking invocations given the DB polling behavior. When user or request is not provided to the activation store, this could signal that a blocking invocation is taking place.
|
@markusthoemmes, very concerned about this polling behavior when actions are invoked. https://github.com/apache/incubator-openwhisk/blob/1d79fa395de32c6a774efeb4f53563689e9cbb7e/core/controller/src/main/scala/whisk/core/controller/actions/PrimitiveActions.scala#L585 |
|
@dubee concerned in which way? I believe this is the database fallback polling in case an active ack goes missing. |
|
@dubee Can you share some more details on how Elasticsearch implementation would make use of user and request data. Would it like to use any aspect of request or we can extract explicit parts from request and pass it on. For now it appears that passing request param is making SPI usage more coupled to web requests |
|
@chetanmeh, certain Elasticsearch deployments require additional authentication which can be passed through the controller via HTTP headers. This allows the controller to communicate to Elasitcsearch for users making requests. Currently our Elasticsearch log store works in the same way. When required headers are specified via the OpenWhisk Ansible configuration for the log store, the log store will pass those headers from the client to Elasticsearch to perform a query. See the log store for example: |
|
@dubee Now its more clear. So we pass on some sort of "user context" to ES. May be we make that notion explicit via having a |
dubee
force-pushed
the
activation-store-user-request
branch
from
0140629
to
0a1dfd0
Compare
|
PG2 3475 🔵 |
|
@markusthoemmes, anything else on this one? |
dubee
added
the
awaits-reviewer
|
@chetanmeh, where do you think the UserContext case class should live? |
dubee
force-pushed
the
activation-store-user-request
branch
2 times, most recently
from
0d7d0b4
to
dd33c35
Compare
|
Added UserContext in the third commit. The case class definition should probably be moved elsewhere though. |
|
PG4 2057 🔵 |
Adding it to |
|
@markusthoemmes, any other comments on this one? |
|
@mdeuser, can you review this one? |
| @@ -29,6 +28,7 @@ import whisk.common.TransactionId | |||
| import whisk.core.containerpool.Container | |||
| import whisk.core.entity.{ActivationLogs, ExecutableWhiskAction, Identity, WhiskActivation} | |||
| import whisk.http.Messages | |||
| import whisk.core.database.UserContext | |||
There was a problem hiding this comment.
is this class tightly coupled with the database package? on the surface, it seems like it could go into some place else.
There was a problem hiding this comment.
@mdeuser, the ActivationStore was moved to the database package recently. Where do you think UserContext should live?
There was a problem hiding this comment.
not entirely sure, but it seems like it is not tightly coupled with the database.. whisk.core.entity ?
| @@ -38,8 +41,9 @@ trait ActivationStore { | |||
| * @param notifier cache change notifier | |||
| * @return Future containing DocInfo related to stored activation | |||
| */ | |||
| def store(activation: WhiskActivation)(implicit transid: TransactionId, | |||
| notifier: Option[CacheChangeNotification]): Future[DocInfo] | |||
| def store(activation: WhiskActivation, context: UserContext)( | |||
There was a problem hiding this comment.
update api comment with new param
| @@ -48,7 +52,7 @@ trait ActivationStore { | |||
| * @param transid transaction ID for request | |||
| * @return Future containing the retrieved WhiskActivation | |||
| */ | |||
| def get(activationId: ActivationId)(implicit transid: TransactionId): Future[WhiskActivation] | |||
| def get(activationId: ActivationId, context: UserContext)(implicit transid: TransactionId): Future[WhiskActivation] | |||
There was a problem hiding this comment.
update api comment with new param
| @@ -58,8 +62,9 @@ trait ActivationStore { | |||
| * @param notifier cache change notifier | |||
| * @return Future containing a Boolean value indication whether the activation was deleted | |||
| */ | |||
| def delete(activationId: ActivationId)(implicit transid: TransactionId, | |||
| notifier: Option[CacheChangeNotification]): Future[Boolean] | |||
| def delete(activationId: ActivationId, context: UserContext)( | |||
There was a problem hiding this comment.
update api comment with new param
| @@ -76,7 +81,8 @@ trait ActivationStore { | |||
| name: Option[EntityPath] = None, | |||
| skip: Int, | |||
| since: Option[Instant] = None, | |||
| upto: Option[Instant] = None)(implicit transid: TransactionId): Future[JsObject] | |||
| upto: Option[Instant] = None, | |||
| context: UserContext)(implicit transid: TransactionId): Future[JsObject] | |||
There was a problem hiding this comment.
update api comment with new param
| includeDocs: Boolean = false, | ||
| since: Option[Instant] = None, | ||
| upto: Option[Instant] = None, | ||
| context: UserContext)(implicit transid: TransactionId): Future[Either[List[JsObject], List[WhiskActivation]]] |
There was a problem hiding this comment.
update api comment with new param
| @@ -517,6 +518,8 @@ protected[actions] trait PrimitiveActions { | |||
| private def completeActivation(user: Identity, session: Session, response: ActivationResponse)( | |||
| implicit transid: TransactionId): WhiskActivation = { | |||
|
|
|||
| val context = UserContext(user) | |||
There was a problem hiding this comment.
is the original request context needed here?
There was a problem hiding this comment.
Yes, the user information needs to be passed to the ActivationStore store() method. This allows ActivationStore SPIs to extract user information if need be.
There was a problem hiding this comment.
but what about the original request context (i.e. request headers, payload, etc).. since there's is a write to the activation store (like in other places) i was thinking the request context would also be needed (like in other places)
There was a problem hiding this comment.
I don't have a use case for passing the request to the activation store here. Would rather keep the invocation path lean unless we really need the HTTP header in the store method.
dubee
force-pushed
the
activation-store-user-request
branch
from
dd33c35
to
1044792
Compare
…ache#3798) * Allow for activation store to accept user and request information * Make user and request non-optional parameters * Introduce UserContext to activation store * Update doc for new parameters
Other ActivationStore implementations, Elasticsearch for instance, will need user and request information.
Description
Related issue and scope
My changes affect the following components
Types of changes
Checklist: