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:
[email protected]
