[GitHub] [openwhisk] rabbah commented on a change in pull request #4885: Disable db record store for successful blocking activations

[GitBox <gi...@apache.org>]

rabbah commented on a change in pull request #4885:
URL: https://github.com/apache/openwhisk/pull/4885#discussion_r412231859



##########
File path: common/scala/src/main/resources/application.conf
##########
@@ -467,10 +471,11 @@ whisk {
         #    log-timestamp-field = "log_timestamp" #splunk field where timestamp is stored (to reflect log event generated time, not splunk's _time)
         #    log-stream-field = "log_stream"       #splunk field where stream is stored (stdout/stderr)
         #    log-message-field = "log_message"     #splunk field where log message is stored
+        #    namespace-field = "namespace"         #splunk field where namespace is stored
         #    activation-id-field = "activation_id" #splunk field where activation id is stored
         #    query-constraints = ""                #additional constraints for splunk queries
-        #    query-timestamp-offset-seconds = ""   #splunk query will be broadened by this 2*<offset value>; e.g. "earliest_time=activation.start - offset" and "latest_time=activation.end + offset"
-        #    disableSNI = false                    #if true, disables hostname validation and cert validation (in case splunk api endpoint is using a self signed cert)
+        #    query-timestamp-offset-seconds = 2   #splunk query will be broadened by this 2*<offset value>; e.g. "earliest_time=activation.start - offset" and "latest_time=activation.end + offset"

Review comment:
       ```suggestion
           #    query-timestamp-offset-seconds = 2    #splunk query will be broadened by this 2*<offset value>; e.g. "earliest_time=activation.start - offset" and "latest_time=activation.end + offset"
   ```

##########
File path: tests/src/test/scala/org/apache/openwhisk/core/controller/test/ControllerTestCommon.scala
##########
@@ -73,7 +73,7 @@ protected trait ControllerTestCommon
   override lazy val entitlementProvider: EntitlementProvider =
     new LocalEntitlementProvider(whiskConfig, loadBalancer, instance)
 
-  override val activationIdFactory = new ActivationId.ActivationIdGenerator() {
+  override val activationIdFactory: ActivationId.ActivationIdGenerator = new ActivationId.ActivationIdGenerator {

Review comment:
       when the type is obvious (it's on the RHS) i think it's ok to omit.
   ```suggestion
     override val activationIdFactory = new ActivationId.ActivationIdGenerator {
   ```

##########
File path: common/scala/src/main/scala/org/apache/openwhisk/core/database/ActivationStore.scala
##########
@@ -154,6 +162,23 @@ trait ActivationStore {
     since: Option[Instant] = None,
     upto: Option[Instant] = None,
     context: UserContext)(implicit transid: TransactionId): Future[Either[List[JsObject], List[WhiskActivation]]]
+
+  /**
+   * Check if the system is configured to not store the activation in the database
+   * Dont't store if activation is successful, blocking, not in debug mode and no disable store is configured

Review comment:
       Trying to understand this, I found it easier to apply De Morgan's law :) hence I suggest
   ```suggestion
      * Only stores activations if one of these is true:
      * - result is an error,
      * - a non-blocking activation
      * - an activation in debug mode
      * - activation stores is not disabled via a configuration parameter
   ```

##########
File path: common/scala/src/main/scala/org/apache/openwhisk/core/database/ActivationStore.scala
##########
@@ -154,6 +162,23 @@ trait ActivationStore {
     since: Option[Instant] = None,
     upto: Option[Instant] = None,
     context: UserContext)(implicit transid: TransactionId): Future[Either[List[JsObject], List[WhiskActivation]]]
+
+  /**
+   * Check if the system is configured to not store the activation in the database

Review comment:
       nit
   
   ```suggestion
      * Checks if the system is configured to not store the activation in the database.
   ```

##########
File path: common/scala/src/main/scala/org/apache/openwhisk/core/database/ActivationStore.scala
##########
@@ -154,6 +162,23 @@ trait ActivationStore {
     since: Option[Instant] = None,
     upto: Option[Instant] = None,
     context: UserContext)(implicit transid: TransactionId): Future[Either[List[JsObject], List[WhiskActivation]]]
+
+  /**
+   * Check if the system is configured to not store the activation in the database
+   * Dont't store if activation is successful, blocking, not in debug mode and no disable store is configured
+   *
+   * @param isSuccess is successful activation
+   * @param isBlocking is blocking activation
+   * @param debugMode is logging header set to "on" for the invocation
+   * @param disableStore is disable store configured
+   * @return Should the activation be stored to the database
+   */
+  private def shouldStoreActivation(isSuccess: Boolean,
+                                    isBlocking: Boolean,
+                                    debugMode: Boolean,
+                                    disableStore: Boolean): Boolean = {
+    !(isSuccess && isBlocking && !debugMode && disableStore)

Review comment:
       As noted above, I think the alternate with ! and || might be clearer. A nit so defer to you.

##########
File path: core/controller/src/main/scala/org/apache/openwhisk/core/controller/actions/PrimitiveActions.scala
##########
@@ -575,7 +578,11 @@ protected[actions] trait PrimitiveActions {
       }
     }
 
-    activationStore.storeAfterCheck(activation, context)(transid, notifier = None)
+    activationStore.storeAfterCheck(activation, context)(
+      transid,
+      notifier = None,
+      blockingComposition,
+      disableStoreResultConfig)

Review comment:
       why is this configuration read in multiple places and passed to the underlying store? Can't it be ready just one?
   The override is for debug mode and I think you can achieve this by having the following states:
   
   - pass None: default pure config applies
   - pass Some(t): t applies
   
   The override uses Some(t) else None.

##########
File path: core/controller/src/main/scala/org/apache/openwhisk/core/controller/ApiUtils.scala
##########
@@ -195,6 +201,72 @@ 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: A => Future[JsObject])(implicit transid: TransactionId, format: RootJsonFormat[A], ma: Manifest[A]) = {
+    onComplete(entity) {
+      case Success(entity) =>
+        logging.debug(this, s"[PROJECT] entity success")
+        onComplete(project(entity)) {
+          case Success(response: JsObject) =>
+            complete(OK, response)
+          case Failure(t: Throwable) =>
+            logging.error(this, s"[PROJECT] projection failed: ${t.getMessage}")
+            terminate(InternalServerError, t.getMessage)
+        }
+      case Failure(t: NoDocumentException) =>
+        // In case disableStoreResult configuration is active, persevere
+        // log might still be available even if entity was not
+        if (disableStoreResultConfig) {
+          val namespace = docId.asString.split("/")(0)
+          val id = docId.asString.split("/")(1)
+          val whiskActivation = WhiskActivation(

Review comment:
       The activation record is being filled with bogus values and at face value this just looks wrong. The reason you're doing this is that the caller will only project the logs property. Since you've already cloned the method to handle the logs, I'd move the projection of the logs field here and eschew creating a bogus activation record which I think is cleaner and safer.




----------------------------------------------------------------
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:
users@infra.apache.org