[GitHub] [openwhisk] rabbah commented on a change in pull request #5169: add system config options for success / failure levels to write blocking / non-blocking activations to db

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

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



##########
File path: common/scala/src/main/scala/org/apache/openwhisk/core/database/ActivationStore.scala
##########
@@ -183,17 +191,29 @@ trait ActivationStore {
    * - an activation in debug mode
    * - activation stores is not disabled via a configuration parameter
    *
-   * @param isSuccess is successful activation
+   * @param activation to check
    * @param isBlocking is blocking activation
    * @param debugMode is logging header set to "on" for the invocation
-   * @param disableStore is disable store configured
+   * @param blockingStoreLevel level of activation status to store for blocking invocations
+   * @param nonBlockingStoreLevel level of activation status to store for blocking invocations
    * @return Should the activation be stored to the database
    */
-  private def shouldStoreActivation(isSuccess: Boolean,
+  private def shouldStoreActivation(activation: WhiskActivation,
                                     isBlocking: Boolean,
                                     debugMode: Boolean,
-                                    disableStore: Boolean): Boolean = {
-    !isSuccess || !isBlocking || debugMode || !disableStore
+                                    blockingStoreLevel: ActivationStoreLevel.Value,
+                                    nonBlockingStoreLevel: ActivationStoreLevel.Value): Boolean = {
+    def shouldStoreOnLevel(storageLevel: ActivationStoreLevel.Value): Boolean = {
+      storageLevel match {
+        case ActivationStoreLevel.STORE_ALWAYS   => true
+        case ActivationStoreLevel.STORE_FAILURES => !activation.response.isSuccess
+        case ActivationStoreLevel.STORE_FAILURES_NOT_APPLICATION_ERRORS =>
+          !activation.response.isSuccess && !activation.response.isApplicationError

Review comment:
       how about turning this into the two failure modes instead to avoid potential double negations.

##########
File path: common/scala/src/main/scala/org/apache/openwhisk/core/database/ActivationStore.scala
##########
@@ -183,17 +191,29 @@ trait ActivationStore {
    * - an activation in debug mode
    * - activation stores is not disabled via a configuration parameter
    *
-   * @param isSuccess is successful activation
+   * @param activation to check
    * @param isBlocking is blocking activation
    * @param debugMode is logging header set to "on" for the invocation
-   * @param disableStore is disable store configured
+   * @param blockingStoreLevel level of activation status to store for blocking invocations
+   * @param nonBlockingStoreLevel level of activation status to store for blocking invocations
    * @return Should the activation be stored to the database
    */
-  private def shouldStoreActivation(isSuccess: Boolean,
+  private def shouldStoreActivation(activation: WhiskActivation,

Review comment:
       you don't need the entire activation, just the activation.response so I'd tighten this param's type.




-- 
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.

To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org