Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Save waitTime value for sequence and conductor action (allow parent/child transaction ids) #4819

Merged
merged 10 commits into from
Jun 3, 2020

Conversation

upgle
Copy link
Member

@upgle upgle commented Feb 10, 2020

Description

Currently, The openwhisk uses the waitTime annotation to store the internal wait time until the action code is provisioned. But this value is only present for non-sequence related activations.

Internally invoked actions can't know waitTime because this value is measured based on the start value of the first created TransactionId by topmost action.

So those actions invoked by topmost action must have a different TransactionId with a different start value.

To solve this problem I've modified the TransactionId case class to have a parent/child structure.

Define the starting point for internally invoked action

This is the invocation flow of the sequence and conductor action in the controller.
(Each action calls internal actions in a different way, so the starting point is different.)

Sequence action

An invokeNextAction method could be the starting point to create child transactionId.

image

wsk action create sequence --sequence increment,increment,increment
[05:04:08.704Z] [#tid_cWCsYNy] [ActionsApi] invokeAction sequence
[05:04:08.705Z] [#tid_cWCsYNy] [ActionsApi] invokeNextAction increment <- create child
[05:04:08.705Z] [#tid_cWCsYNy] [ActionsApi] invokeAction increment
[05:04:08.705Z] [#tid_cWCsYNy] [ActionsApi] invokeSingleAction increment
[05:04:08.705Z] [#tid_cWCsYNy] [ActionsApi] invokeSimpleAction increment
[05:04:08.807Z] [#tid_cWCsYNy] [ActionsApi] invokeNextAction increment  <- create child
[05:04:08.807Z] [#tid_cWCsYNy] [ActionsApi] invokeAction increment
[05:04:08.807Z] [#tid_cWCsYNy] [ActionsApi] invokeSingleAction increment
[05:04:08.807Z] [#tid_cWCsYNy] [ActionsApi] invokeSimpleAction increment
[05:04:08.828Z] [#tid_cWCsYNy] [ActionsApi] invokeNextAction increment  <- create child
[05:04:08.828Z] [#tid_cWCsYNy] [ActionsApi] invokeAction increment
[05:04:08.828Z] [#tid_cWCsYNy] [ActionsApi] invokeSingleAction increment
[05:04:08.828Z] [#tid_cWCsYNy] [ActionsApi] invokeSimpleAction increment

Conductor action

A tryInvokeNext and invokeConductor method could be the starting point to create child transactionId.

image

[05:05:55.575Z] [#tid_oVi9Qxc] [ActionsApi] invokeAction tripleAndIncrement
[05:05:55.575Z] [#tid_oVi9Qxc] [ActionsApi] invokeSingleAction tripleAndIncrement
[05:05:55.575Z] [#tid_oVi9Qxc] [ActionsApi] invokeComposition tripleAndIncrement
[05:05:55.577Z] [#tid_oVi9Qxc] [ActionsApi] invokeConductor Some({})  <- create child
[05:05:55.578Z] [#tid_oVi9Qxc] [ActionsApi] ┗ invokeSimpleAction tripleAndIncrement
[05:05:55.720Z] [#tid_oVi9Qxc] [ActionsApi] ┗ tryInvokeNext whisk.system/triple  <- create child
[05:05:55.728Z] [#tid_oVi9Qxc] [ActionsApi]    ┗ invokeComponent triple
[05:05:55.728Z] [#tid_oVi9Qxc] [ActionsApi]        ┗ invokeSimpleAction triple
[05:05:55.879Z] [#tid_oVi9Qxc] [ActionsApi] invokeConductor Some({"value":null})  <- create child
[05:05:55.880Z] [#tid_oVi9Qxc] [ActionsApi] ┗ invokeSimpleAction tripleAndIncrement
[05:05:55.897Z] [#tid_oVi9Qxc] [ActionsApi] ┗ tryInvokeNext whisk.system/increment  <- create child
[05:05:55.902Z] [#tid_oVi9Qxc] [ActionsApi]   ┗ invokeComponent increment
[05:05:55.902Z] [#tid_oVi9Qxc] [ActionsApi]       ┗ invokeSimpleAction increment
[05:05:56.771Z] [#tid_oVi9Qxc] [ActionsApi] invokeConductor Some({"value":1})  <- create child
[05:05:56.771Z] [#tid_oVi9Qxc] [ActionsApi] ┗ invokeSimpleAction tripleAndIncrement

Creating a new child TransactionId

You can create a new child transaction by calling the childOf method of TransactionId.

  private def tryInvokeNext(user: Identity,
                            fqn: FullyQualifiedEntityName,
                            params: Option[JsObject],
                            session: Session,
                            parentTid: TransactionId): Future[ActivationResponse] = {

    implicit val transid: TransactionId = TransactionId.childOf(parentTid)

Print root transaction id and leaf transaction id together

image

Related issue and scope

#3083

My changes affect the following components

  • TransactionId
  • Intrinsic actions (e.g., sequences, conductors)
  • Documentation

Types of changes

  • Enhancement or new feature (adds new functionality).
  • Breaking change (a bug fix or enhancement which changes existing behavior).

Checklist:

  • I signed an Apache CLA.
  • I reviewed the style guides and followed the recommendations (Travis CI will check :).
  • I added tests to cover my changes.
  • My changes require further changes to the documentation.
  • I updated the documentation where necessary.

@upgle upgle changed the title Save waitTime value for sequence and conductor action (allow parent/child transaction ids) WIP: Save waitTime value for sequence and conductor action (allow parent/child transaction ids) Feb 10, 2020
@codecov-io
Copy link

codecov-io commented Feb 10, 2020

Codecov Report

Merging #4819 into master will decrease coverage by 6.59%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #4819     +/-   ##
=========================================
- Coverage   85.12%   78.52%   -6.6%     
=========================================
  Files         197      197             
  Lines        8879     8905     +26     
  Branches      607      610      +3     
=========================================
- Hits         7558     6993    -565     
- Misses       1321     1912    +591
Impacted Files Coverage Δ
...la/org/apache/openwhisk/common/TransactionId.scala 94.87% <100%> (+1.32%) ⬆️
...in/scala/org/apache/openwhisk/common/Logging.scala 74.3% <100%> (-9.32%) ⬇️
.../openwhisk/core/containerpool/ContainerProxy.scala 91.89% <100%> (-0.09%) ⬇️
...hisk/core/controller/actions/SequenceActions.scala 92.37% <100%> (+0.13%) ⬆️
...isk/core/controller/actions/PrimitiveActions.scala 88.05% <100%> (+0.8%) ⬆️
...a/org/apache/openwhisk/http/BasicHttpService.scala 93.1% <100%> (-0.12%) ⬇️
...core/database/cosmosdb/RxObservableImplicits.scala 0% <0%> (-100%) ⬇️
...enwhisk/connector/kafka/KamonMetricsReporter.scala 0% <0%> (-100%) ⬇️
...e/database/cosmosdb/cache/ChangeFeedConsumer.scala 0% <0%> (-100%) ⬇️
...ore/database/cosmosdb/cache/CacheInvalidator.scala 0% <0%> (-100%) ⬇️
... and 17 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 970d42a...7b13220. Read the comment docs.

@rabbah
Copy link
Member

rabbah commented Feb 10, 2020

Awesome! Thank you for adding this.

@rabbah rabbah self-requested a review February 10, 2020 12:26
@upgle upgle changed the title WIP: Save waitTime value for sequence and conductor action (allow parent/child transaction ids) Save waitTime value for sequence and conductor action (allow parent/child transaction ids) Feb 11, 2020
@upgle
Copy link
Member Author

upgle commented Feb 12, 2020

I've found a performance improvement point for this PR. As you know, using the string builder is much faster than the current version.

The blue line shows the performance of the transaction id generator currently used by openwhisk, and the orange line shows the performance of the implementation using the string builder.

I'll add a new commit.

image

(In the chart, the x-axis is the number of repetitions and the y-axis is the time taken.)

Source code for performance testing

object TransactionId {
  private val dict = ('A' to 'Z') ++ ('a' to 'z') ++ ('0' to '9')

  def generateTidByMap(): String = {
    (0 until 32).map(_ => dict(ThreadLocalRandom.current().nextInt(dict.size))).mkString("")
  }

  def generateTidByStringBuilder(): String = {
    val sb = new StringBuilder
    for (_ <- 1 to 32) {
      sb.append(dict(util.Random.nextInt(dict.length)))
    }
    sb.toString
  }
}

Copy link
Member

@rabbah rabbah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This generally looks good to me. Nice work 👏

*/
@tailrec
private def findRoot(meta: TransactionMetadata): TransactionMetadata =
if (meta.parent.isDefined) findRoot(meta.parent.get) else meta
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@markusthoemmes tailrec work with option map?

*/
@tailrec
private def findRoot(meta: TransactionMetadata): TransactionMetadata =
if (meta.parent.isDefined) findRoot(meta.parent.get) else meta
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (meta.parent.isDefined) findRoot(meta.parent.get) else meta
meta.parent match {
case Some(parent) => findRoot(parent)
case _ => meta

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

avoid .get even though it's safe, also clearer to me

Copy link
Member Author

@upgle upgle May 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rabbah Thank you for your sample code, I've modified the code.

@style95
Copy link
Member

style95 commented May 18, 2020

@upgle Sorry for letting this become stale.
Could you address the conflicts?

@codecov-commenter
Copy link

codecov-commenter commented May 25, 2020

Codecov Report

Merging #4819 into master will decrease coverage by 6.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4819      +/-   ##
==========================================
- Coverage   83.34%   77.33%   -6.01%     
==========================================
  Files         200      201       +1     
  Lines        9334     9469     +135     
  Branches      408      394      -14     
==========================================
- Hits         7779     7323     -456     
- Misses       1555     2146     +591     
Impacted Files Coverage Δ
...in/scala/org/apache/openwhisk/common/Logging.scala 76.64% <100.00%> (-8.17%) ⬇️
...la/org/apache/openwhisk/common/TransactionId.scala 95.12% <100.00%> (+1.47%) ⬆️
...a/org/apache/openwhisk/http/BasicHttpService.scala 93.10% <100.00%> (-0.12%) ⬇️
...isk/core/controller/actions/PrimitiveActions.scala 86.16% <100.00%> (+0.83%) ⬆️
...hisk/core/controller/actions/SequenceActions.scala 80.00% <100.00%> (+0.33%) ⬆️
.../openwhisk/core/containerpool/ContainerProxy.scala 93.80% <100.00%> (+0.23%) ⬆️
...core/database/cosmosdb/RxObservableImplicits.scala 0.00% <0.00%> (-100.00%) ⬇️
...ore/database/cosmosdb/cache/CacheInvalidator.scala 0.00% <0.00%> (-100.00%) ⬇️
...e/database/cosmosdb/cache/ChangeFeedConsumer.scala 0.00% <0.00%> (-100.00%) ⬇️
...core/database/cosmosdb/CosmosDBArtifactStore.scala 0.00% <0.00%> (-96.23%) ⬇️
... and 29 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8ec3c24...3db7e27. Read the comment docs.

@upgle
Copy link
Member Author

upgle commented May 25, 2020

@style95 @rabbah
I've updated this PR, and all test cases have been passed. Please review it again.

Copy link
Member

@style95 style95 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@style95 style95 merged commit a44db4b into apache:master Jun 3, 2020
@jiangpengcheng jiangpengcheng mentioned this pull request Jun 29, 2020
21 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants