Introduce scheduling configurations. #5232

style95 · 2022-05-10T12:49:33Z

Description

This is to make a couple of scheduling factors configurable.

Related issue and scope

I opened an issue to propose and discuss this change (#????)

My changes affect the following components

Types of changes

Bug fix (generally a non-breaking change which closes an issue).
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.

...duler/src/main/scala/org/apache/openwhisk/core/scheduler/queue/SchedulingDecisionMaker.scala

style95 · 2022-05-10T12:52:41Z

tests/src/test/scala/org/apache/openwhisk/core/invoker/test/DefaultInvokerServerTests.scala

-      Unmarshal(responseEntity).to[String].map(response => {
-        InvokerEnabled.parseJson(response) shouldEqual InvokerEnabled(true)
-      })
+      Unmarshal(responseEntity)


I am unclear why scalaFmt suddenly requires this kind of change.

style95 · 2022-05-10T12:53:42Z

core/scheduler/src/main/scala/org/apache/openwhisk/core/scheduler/queue/MemoryQueue.scala

@@ -116,7 +112,7 @@ class MemoryQueue(private val etcdClient: EtcdClient,
                  private val durationChecker: DurationChecker,
                  private val action: FullyQualifiedEntityName,
                  messagingProducer: MessageProducer,
-                  config: WhiskConfig,


Seems this is no longer being used.

bdoyle0182 · 2022-05-10T17:27:55Z

core/scheduler/src/main/resources/application.conf

+    scheduling {
+      stale-threshold = "100 milliseconds"
+      check-interval = "100 milliseconds"
+      drop-interval = "10 seconds"


This is another config I forgot to mention we'll need to tune so good to see it configurable here. Does 10 seconds make sense as the default if the built in max action duration is 60 seconds? If the namespace or action is throttled and can't create more containers, then may be waiting up to 60 seconds to process new activations. On the flip side I think this is used to be able to stop the scheduler once all queues are drained before stopping so I also understand why you would want this to be on the lower side

The drop interval is to periodically check whether there are any stale activations.
https://github.com/apache/openwhisk/blob/master/core/scheduler/src/main/scala/org/apache/openwhisk/core/scheduler/queue/MemoryQueue.scala#L865

The retention time is controlled by the queue retention time which is 60s by default.
https://github.com/apache/openwhisk/blob/master/core/scheduler/src/main/resources/application.conf#L73

I see okay. Maybe the max retention ms default should be the same as the default completion ack timeout which I think is three minutes based on the timeout calculation in code

ok, so do you want me to update the default retention time to 3 mins?

Yea I think that would be best unless you see a reason why it should be 1 min since you're more familiar with the architecture

bdoyle0182 · 2022-05-10T17:28:56Z

core/scheduler/src/main/scala/org/apache/openwhisk/core/scheduler/queue/MemoryQueue.scala

@@ -911,7 +902,7 @@ class MemoryQueue(private val etcdClient: EtcdClient,
  private def getStaleActivationNum(count: Int, queue: Queue[TimeSeriesActivationEntry]): Int = {
    if (queue.isEmpty || Duration
          .between(queue.head.timestamp, Instant.now)
-          .compareTo(StaleDuration) < 0) count
+          .compareTo(schedulingConfig.staleThreshold.toJava) < 0) count


why is toJava needed here?

The FiniteDuration of scala does not extend Comparable.
Since it compares duration based on Duration of java, it requires that.

bdoyle0182 · 2022-05-10T20:14:27Z

core/scheduler/src/main/resources/application.conf

@@ -64,6 +64,11 @@ whisk {
    grpc {
      tls = "false"
    }
+    scheduling {


since this is already under scheduler I think this isn't providing any more descriptiveness. How about calling the config decision-maker?

These configurations are related to both MemoryQueue and DecisionMaker.
The two intervals are not used by the decision-maker.

bdoyle0182 · 2022-05-10T20:17:19Z

ansible/group_vars/all

+  scheduling:
+    staleThreshold: "{{ scheduler_scheduling_staleThreshold | default('100 milliseconds') }}"
+    checkInterval: "{{ scheduler_scheduling_checkInterval | default('100 milliseconds') }}"
+    dropInterval: "{{ scheduler_scheduling_dropInterval | default('10 seconds') }}"


How about dropStaleActivationTime for this config? Or is the config here the interval at which to check whether there are stale activations to drop

This is just an interval to check to see if there are any stale activations.

ningyougang · 2022-05-11T01:12:39Z

core/scheduler/src/main/resources/application.conf

@@ -64,6 +64,11 @@ whisk {
    grpc {
      tls = "false"
    }
+    scheduling {
+      stale-threshold = "100 milliseconds"


Because the value is FiniteDuration type, how about change it to stale-duration-threshold?
(btw, check-interval or drop-interval, from the name, we can know it is a FiniteDuration type easily)

I just thought its value includes the unit so we can easily figure out its type.

bdoyle0182 · 2022-05-18T19:12:05Z

LGTM

jiangpengcheng · 2022-05-19T00:43:04Z

looks like there is a compile error

style95 · 2022-05-19T01:25:50Z

It's surprising that scalaFmt could not detect the error.

> Task :core:scheduler:compileScala
Pruning sources from previous analysis, due to incompatible CompileSetup.
/Users/style95/git/openwhisk/core/scheduler/src/main/scala/org/apache/openwhisk/core/scheduler/queue/MemoryQueue.scala:58: Unused import
import scala.language.postfixOps
                      ^
one error found

> Task :core:scheduler:compileScala FAILED

FAILURE: Build failed with an exception.

I've removed the unused import.

bdoyle0182 · 2022-05-27T17:25:45Z

Is it just the build blocking this right now?

style95 · 2022-05-29T07:55:09Z

Is it just the build blocking this right now?

I think so.

style95 · 2022-05-30T00:51:15Z

It's weird that Travis complains about what I have not changed such as Logging.scala.

/home/travis/build/apache/openwhisk/common/scala/src/main/scala/org/apache/openwhisk/common/Logging.scala:311: Widening conversion from Long to Double is deprecated because it loses precision. Write `.toDouble` instead.
      token.gauge.update(value)

codecov-commenter · 2022-05-30T05:22:54Z

Codecov Report

Attention: Patch coverage is 57.89474% with 8 lines in your changes missing coverage. Please review.

Project coverage is 75.18%. Comparing base (1e09049) to head (3197612).
Report is 139 commits behind head on master.

Files with missing lines	Patch %	Lines
.../openwhisk/core/loadBalancer/FPCPoolBalancer.scala	0.00%	5 Missing ⚠️
...rg/apache/openwhisk/core/scheduler/Scheduler.scala	0.00%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5232      +/-   ##
==========================================
- Coverage   80.16%   75.18%   -4.99%     
==========================================
  Files         238      238              
  Lines       14009    14032      +23     
  Branches      567      578      +11     
==========================================
- Hits        11231    10550     -681     
- Misses       2778     3482     +704

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

style95 · 2022-05-30T05:24:32Z

common/scala/build.gradle

@@ -63,8 +63,6 @@ dependencies {
    compile "com.google.code.findbugs:jsr305:3.0.2"
    compile "io.fabric8:kubernetes-client:${gradle.kube_client.version}"

-    compile "org.scala-lang.modules:scala-java8-compat_${gradle.scala.depVersion}:1.0.2"


I reverted this change as it introduces a compatibility issue.

bdoyle0182 · 2022-05-31T18:18:54Z

Looks like the build passed, I'm good with merging in

* Introduce scheduling configurations. * Apply SchedulingConfig to MemoryQueue. * Apply SchedulingConfig to SchedulingDecisionMaker. * Apply ScalaFmt * Remove unused import * Change configs. * Fix test cases. * Apply scalaFmt * Remove Java8-compat dependency.

style95 requested review from bdoyle0182 and jiangpengcheng May 10, 2022 12:49

style95 commented May 10, 2022

View reviewed changes

bdoyle0182 reviewed May 10, 2022

View reviewed changes

ningyougang reviewed May 11, 2022

View reviewed changes

bdoyle0182 approved these changes May 18, 2022

View reviewed changes

style95 force-pushed the configurable-pause-grace branch from 1905e03 to 6c08803 Compare May 23, 2022 12:19

style95 added 8 commits May 29, 2022 16:49

Introduce scheduling configurations.

3b50920

Apply SchedulingConfig to MemoryQueue.

e249318

Apply SchedulingConfig to SchedulingDecisionMaker.

e848130

Apply ScalaFmt

63a3f0a

Remove unused import

1584d97

Change configs.

cbab6ce

Fix test cases.

2e585c5

Apply scalaFmt

2e82d87

style95 force-pushed the configurable-pause-grace branch from db247e0 to 2e82d87 Compare May 29, 2022 07:54

Remove Java8-compat dependency.

3197612

style95 commented May 30, 2022

View reviewed changes

style95 merged commit 4ef94d7 into apache:master May 31, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce scheduling configurations. #5232

Introduce scheduling configurations. #5232

style95 commented May 10, 2022

style95 May 10, 2022

style95 May 10, 2022

bdoyle0182 May 10, 2022

style95 May 11, 2022

bdoyle0182 May 24, 2022

style95 May 25, 2022

bdoyle0182 May 25, 2022

bdoyle0182 May 10, 2022

style95 May 11, 2022

bdoyle0182 May 10, 2022

style95 May 11, 2022

bdoyle0182 May 10, 2022

style95 May 11, 2022

ningyougang May 11, 2022 •

edited

Loading

style95 May 11, 2022

bdoyle0182 commented May 18, 2022

jiangpengcheng commented May 19, 2022

style95 commented May 19, 2022

bdoyle0182 commented May 27, 2022

style95 commented May 29, 2022

style95 commented May 30, 2022

codecov-commenter commented May 30, 2022 •

edited

Loading

style95 May 30, 2022

bdoyle0182 commented May 31, 2022

Introduce scheduling configurations. #5232

Introduce scheduling configurations. #5232

Conversation

style95 commented May 10, 2022

Description

Related issue and scope

My changes affect the following components

Types of changes

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ningyougang May 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bdoyle0182 commented May 18, 2022

jiangpengcheng commented May 19, 2022

style95 commented May 19, 2022

bdoyle0182 commented May 27, 2022

style95 commented May 29, 2022

style95 commented May 30, 2022

codecov-commenter commented May 30, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

bdoyle0182 commented May 31, 2022

ningyougang May 11, 2022 •

edited

Loading

codecov-commenter commented May 30, 2022 •

edited

Loading