rabbah commented on a change in pull request #4885:
URL: https://github.com/apache/openwhisk/pull/4885#discussion_r414917419
##########
File path:
common/scala/src/main/scala/org/apache/openwhisk/core/containerpool/logging/DockerToActivationLogStore.scala
##########
@@ -68,8 +68,16 @@ class DockerToActivationLogStore(system: ActorSystem)
extends LogStore {
override val containerParameters = Map("--log-driver" -> Set("json-file"))
/* As logs are already part of the activation record, just return that bit
of it */
- override def fetchLogs(activation: WhiskActivation, context: UserContext):
Future[ActivationLogs] =
- Future.successful(activation.logs)
+ override def fetchLogs(namespace: String,
+ activationId: String,
Review comment:
we have a type `ActivationId` instead of `string` - is it more
convenient to use string here?
##########
File path:
common/scala/src/main/scala/org/apache/openwhisk/core/database/ActivationStore.scala
##########
@@ -24,30 +24,51 @@ import akka.stream.ActorMaterializer
import akka.http.scaladsl.model.HttpRequest
import spray.json.JsObject
import org.apache.openwhisk.common.{Logging, TransactionId}
+import org.apache.openwhisk.core.ConfigKeys
import org.apache.openwhisk.core.entity._
import org.apache.openwhisk.spi.Spi
+import pureconfig.loadConfigOrThrow
import scala.concurrent.Future
case class UserContext(user: Identity, request: HttpRequest = HttpRequest())
trait ActivationStore {
+ protected val disableStoreResultConfig =
loadConfigOrThrow[Boolean](ConfigKeys.disableStoreResult)
+ protected val unstoredLogsEnabledConfig =
loadConfigOrThrow[Boolean](ConfigKeys.unstoredLogsEnabled)
+
/**
* Checks if an activation should be stored in database and stores it.
*
* @param activation activation to store
+ * @param isBlockingActivation is activation blocking
* @param context user and request context
* @param transid transaction ID for request
* @param notifier cache change notifier
* @return Future containing DocInfo related to stored activation
*/
- def storeAfterCheck(activation: WhiskActivation, context: UserContext)(
- implicit transid: TransactionId,
- notifier: Option[CacheChangeNotification]): Future[DocInfo] = {
- if (context.user.limits.storeActivations.getOrElse(true)) {
+ def storeAfterCheck(activation: WhiskActivation,
+ isBlockingActivation: Boolean,
+ disableStore: Option[Boolean],
+ context: UserContext)(implicit transid: TransactionId,
+ notifier:
Option[CacheChangeNotification],
+ logging: Logging): Future[DocInfo]
= {
+ if (context.user.limits.storeActivations.getOrElse(true) &&
+ shouldStoreActivation(
+ activation.response.isSuccess,
+ isBlockingActivation,
+ transid.meta.extraLogging,
+ disableStore.getOrElse(disableStoreResultConfig))) {
+
store(activation, context)
} else {
+ if (unstoredLogsEnabledConfig) {
+ logging.info(
+ this,
+ s"Successful NOT stored activation
${activation.activationId.asString} for action ${activation.name} from
namespace ${activation.namespace.asString} with
response_size=${activation.response.size
Review comment:
nit, tweaking log message - the first reads weirdly to me.
##########
File path:
common/scala/src/main/scala/org/apache/openwhisk/core/database/ActivationStore.scala
##########
@@ -24,30 +24,51 @@ import akka.stream.ActorMaterializer
import akka.http.scaladsl.model.HttpRequest
import spray.json.JsObject
import org.apache.openwhisk.common.{Logging, TransactionId}
+import org.apache.openwhisk.core.ConfigKeys
import org.apache.openwhisk.core.entity._
import org.apache.openwhisk.spi.Spi
+import pureconfig.loadConfigOrThrow
import scala.concurrent.Future
case class UserContext(user: Identity, request: HttpRequest = HttpRequest())
trait ActivationStore {
+ protected val disableStoreResultConfig =
loadConfigOrThrow[Boolean](ConfigKeys.disableStoreResult)
+ protected val unstoredLogsEnabledConfig =
loadConfigOrThrow[Boolean](ConfigKeys.unstoredLogsEnabled)
+
/**
* Checks if an activation should be stored in database and stores it.
*
* @param activation activation to store
+ * @param isBlockingActivation is activation blocking
* @param context user and request context
* @param transid transaction ID for request
* @param notifier cache change notifier
* @return Future containing DocInfo related to stored activation
*/
- def storeAfterCheck(activation: WhiskActivation, context: UserContext)(
- implicit transid: TransactionId,
- notifier: Option[CacheChangeNotification]): Future[DocInfo] = {
- if (context.user.limits.storeActivations.getOrElse(true)) {
+ def storeAfterCheck(activation: WhiskActivation,
+ isBlockingActivation: Boolean,
+ disableStore: Option[Boolean],
+ context: UserContext)(implicit transid: TransactionId,
+ notifier:
Option[CacheChangeNotification],
+ logging: Logging): Future[DocInfo]
= {
+ if (context.user.limits.storeActivations.getOrElse(true) &&
+ shouldStoreActivation(
+ activation.response.isSuccess,
+ isBlockingActivation,
+ transid.meta.extraLogging,
+ disableStore.getOrElse(disableStoreResultConfig))) {
+
store(activation, context)
} else {
+ if (unstoredLogsEnabledConfig) {
+ logging.info(
+ this,
+ s"Successful NOT stored activation
${activation.activationId.asString} for action ${activation.name} from
namespace ${activation.namespace.asString} with
response_size=${activation.response.size
Review comment:
```suggestion
s"Explicitly NOT storing activation
${activation.activationId.asString} for action ${activation.name} from
namespace ${activation.namespace.asString} with
response_size=${activation.response.size
```
##########
File path:
core/controller/src/main/scala/org/apache/openwhisk/core/controller/Activations.scala
##########
@@ -219,8 +223,15 @@ trait WhiskActivationsApi extends Directives with
AuthenticatedRouteProvider wit
* - 500 Internal Server Error
*/
private def fetchLogs(context: UserContext, docid: DocId)(implicit transid:
TransactionId) = {
- getEntityAndProject(
+ getEntityAndProjectLog(
activationStore.get(ActivationId(docid.asString), context),
- (activation: WhiskActivation) => logStore.fetchLogs(activation,
context).map(_.toJsonObject))
+ docid,
+ disableStoreResultConfig,
+ (namespace: String,
+ activationId: String,
Review comment:
about my earlier comment regarding the type `ActivationId`, notice on
line 223/227 an instance of `ActivationId` is created. I'd rather that's used
everywhere for prudence.
##########
File path:
core/controller/src/main/scala/org/apache/openwhisk/core/controller/ApiUtils.scala
##########
@@ -195,6 +194,75 @@ trait ReadOps extends Directives {
terminate(InternalServerError)
}
}
+
+ /**
+ * Waits on specified Future that returns an entity of type A from datastore.
+ * In case A entity is not stored, use the docId to search logstore
+ * Terminates HTTP request.
+ *
+ * @param entity future that returns an entity of type A fetched from
datastore
+ * @param docId activation DocId
+ * @param disableStoreResultConfig configuration
+ * @param project a function A => JSON which projects fields form A
+ *
+ * Responses are one of (Code, Message)
+ * - 200 project(A) as JSON
+ * - 404 Not Found
+ * - 500 Internal Server Error
+ */
+ protected def getEntityAndProjectLog[A <: DocumentRevisionProvider, Au >: A](
+ entity: Future[A],
+ docId: DocId,
+ disableStoreResultConfig: Boolean,
+ project: (String, String, Option[Instant], Option[Instant],
Option[ActivationLogs]) => Future[JsObject])(
+ implicit transid: TransactionId,
+ format: RootJsonFormat[A],
+ ma: Manifest[A]) = {
+ onComplete(entity) {
+ case Success(entity) =>
+ logging.debug(this, s"[PROJECT] entity success")
+ val activation = entity.asInstanceOf[WhiskActivation]
+ onComplete(
+ project(
+ activation.namespace.asString,
+ activation.activationId.asString,
Review comment:
don't need to asString the action id here either if you make the
suggested type change.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
