You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@openwhisk.apache.org by GitBox <gi...@apache.org> on 2022/05/10 12:49:35 UTC
[GitHub] [openwhisk] style95 opened a new pull request, #5232: Introduce scheduling configurations.
style95 opened a new pull request, #5232:
URL: https://github.com/apache/openwhisk/pull/5232
## Description
This is to make a couple of scheduling factors configurable.
## Related issue and scope
<!--- Please include a link to a related issue if there is one. -->
- [ ] I opened an issue to propose and discuss this change (#????)
## My changes affect the following components
<!--- Select below all system components are affected by your change. -->
<!--- Enter an `x` in all applicable boxes. -->
- [ ] API
- [ ] Controller
- [ ] Message Bus (e.g., Kafka)
- [ ] Loadbalancer
- [x] Scheduler
- [ ] Invoker
- [ ] Intrinsic actions (e.g., sequences, conductors)
- [ ] Data stores (e.g., CouchDB)
- [ ] Tests
- [ ] Deployment
- [ ] CLI
- [ ] General tooling
- [ ] Documentation
## Types of changes
<!--- What types of changes does your code introduce? Use `x` in all the boxes that apply: -->
- [ ] Bug fix (generally a non-breaking change which closes an issue).
- [x] Enhancement or new feature (adds new functionality).
- [ ] Breaking change (a bug fix or enhancement which changes existing behavior).
## Checklist:
<!--- Please review the points below which help you make sure you've covered all aspects of the change you're making. -->
- [x] I signed an [Apache CLA](https://github.com/apache/openwhisk/blob/master/CONTRIBUTING.md).
- [x] I reviewed the [style guides](https://github.com/apache/openwhisk/blob/master/CONTRIBUTING.md#coding-standards) 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.
--
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
[GitHub] [openwhisk] bdoyle0182 commented on a diff in pull request #5232: Introduce scheduling configurations.
Posted by GitBox <gi...@apache.org>.
bdoyle0182 commented on code in PR #5232:
URL: https://github.com/apache/openwhisk/pull/5232#discussion_r869510150
##########
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
Review Comment:
why is `toJava` needed here?
--
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
[GitHub] [openwhisk] style95 commented on a diff in pull request #5232: Introduce scheduling configurations.
Posted by GitBox <gi...@apache.org>.
style95 commented on code in PR #5232:
URL: https://github.com/apache/openwhisk/pull/5232#discussion_r881221156
##########
core/scheduler/src/main/resources/application.conf:
##########
@@ -64,6 +64,11 @@ whisk {
grpc {
tls = "false"
}
+ scheduling {
+ stale-threshold = "100 milliseconds"
+ check-interval = "100 milliseconds"
+ drop-interval = "10 seconds"
Review Comment:
ok, so do you want me to update the default retention time to 3 mins?
--
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
[GitHub] [openwhisk] codecov-commenter commented on pull request #5232: Introduce scheduling configurations.
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #5232:
URL: https://github.com/apache/openwhisk/pull/5232#issuecomment-1140711603
# [Codecov](https://codecov.io/gh/apache/openwhisk/pull/5232?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#5232](https://codecov.io/gh/apache/openwhisk/pull/5232?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (3197612) into [master](https://codecov.io/gh/apache/openwhisk/commit/1e09049aaf32e595d13a67ead2cec583a3b1809d?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (1e09049) will **decrease** coverage by `75.41%`.
> The diff coverage is `10.52%`.
```diff
@@ Coverage Diff @@
## master #5232 +/- ##
==========================================
- Coverage 79.99% 4.58% -75.42%
==========================================
Files 238 238
Lines 14009 14008 -1
Branches 567 578 +11
==========================================
- Hits 11207 642 -10565
- Misses 2802 13366 +10564
```
| [Impacted Files](https://codecov.io/gh/apache/openwhisk/pull/5232?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...isk/core/database/memory/NoopActivationStore.scala](https://codecov.io/gh/apache/openwhisk/pull/5232/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29tbW9uL3NjYWxhL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvb3BlbndoaXNrL2NvcmUvZGF0YWJhc2UvbWVtb3J5L05vb3BBY3RpdmF0aW9uU3RvcmUuc2NhbGE=) | `0.00% <ø> (ø)` | |
| [.../openwhisk/core/loadBalancer/FPCPoolBalancer.scala](https://codecov.io/gh/apache/openwhisk/pull/5232/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29yZS9jb250cm9sbGVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvb3BlbndoaXNrL2NvcmUvbG9hZEJhbGFuY2VyL0ZQQ1Bvb2xCYWxhbmNlci5zY2FsYQ==) | `0.00% <0.00%> (-32.96%)` | :arrow_down: |
| [...rg/apache/openwhisk/core/scheduler/Scheduler.scala](https://codecov.io/gh/apache/openwhisk/pull/5232/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29yZS9zY2hlZHVsZXIvc3JjL21haW4vc2NhbGEvb3JnL2FwYWNoZS9vcGVud2hpc2svY29yZS9zY2hlZHVsZXIvU2NoZWR1bGVyLnNjYWxh) | `0.00% <0.00%> (-10.14%)` | :arrow_down: |
| [...sk/core/scheduler/container/ContainerManager.scala](https://codecov.io/gh/apache/openwhisk/pull/5232/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29yZS9zY2hlZHVsZXIvc3JjL21haW4vc2NhbGEvb3JnL2FwYWNoZS9vcGVud2hpc2svY29yZS9zY2hlZHVsZXIvY29udGFpbmVyL0NvbnRhaW5lck1hbmFnZXIuc2NhbGE=) | `0.00% <0.00%> (-96.90%)` | :arrow_down: |
| [...e/openwhisk/core/scheduler/queue/MemoryQueue.scala](https://codecov.io/gh/apache/openwhisk/pull/5232/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29yZS9zY2hlZHVsZXIvc3JjL21haW4vc2NhbGEvb3JnL2FwYWNoZS9vcGVud2hpc2svY29yZS9zY2hlZHVsZXIvcXVldWUvTWVtb3J5UXVldWUuc2NhbGE=) | `0.00% <0.00%> (-84.06%)` | :arrow_down: |
| [...core/scheduler/queue/SchedulingDecisionMaker.scala](https://codecov.io/gh/apache/openwhisk/pull/5232/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29yZS9zY2hlZHVsZXIvc3JjL21haW4vc2NhbGEvb3JnL2FwYWNoZS9vcGVud2hpc2svY29yZS9zY2hlZHVsZXIvcXVldWUvU2NoZWR1bGluZ0RlY2lzaW9uTWFrZXIuc2NhbGE=) | `0.00% <0.00%> (-98.79%)` | :arrow_down: |
| [.../scala/org/apache/openwhisk/core/WhiskConfig.scala](https://codecov.io/gh/apache/openwhisk/pull/5232/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29tbW9uL3NjYWxhL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvb3BlbndoaXNrL2NvcmUvV2hpc2tDb25maWcuc2NhbGE=) | `67.26% <100.00%> (-28.53%)` | :arrow_down: |
| [.../main/scala/org/apache/openwhisk/core/WarmUp.scala](https://codecov.io/gh/apache/openwhisk/pull/5232/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29tbW9uL3NjYWxhL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvb3BlbndoaXNrL2NvcmUvV2FybVVwLnNjYWxh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ain/scala/org/apache/openwhisk/spi/SpiLoader.scala](https://codecov.io/gh/apache/openwhisk/pull/5232/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29tbW9uL3NjYWxhL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvb3BlbndoaXNrL3NwaS9TcGlMb2FkZXIuc2NhbGE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...n/scala/org/apache/openwhisk/utils/JsHelpers.scala](https://codecov.io/gh/apache/openwhisk/pull/5232/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29tbW9uL3NjYWxhL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvb3BlbndoaXNrL3V0aWxzL0pzSGVscGVycy5zY2FsYQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| ... and [208 more](https://codecov.io/gh/apache/openwhisk/pull/5232/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/openwhisk/pull/5232?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/openwhisk/pull/5232?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [1e09049...3197612](https://codecov.io/gh/apache/openwhisk/pull/5232?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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
[GitHub] [openwhisk] bdoyle0182 commented on a diff in pull request #5232: Introduce scheduling configurations.
Posted by GitBox <gi...@apache.org>.
bdoyle0182 commented on code in PR #5232:
URL: https://github.com/apache/openwhisk/pull/5232#discussion_r869511258
##########
core/scheduler/src/main/scala/org/apache/openwhisk/core/scheduler/queue/SchedulingDecisionMaker.scala:
##########
@@ -3,16 +3,19 @@ package org.apache.openwhisk.core.scheduler.queue
import akka.actor.{Actor, ActorSystem, Props}
import org.apache.openwhisk.common.Logging
import org.apache.openwhisk.core.entity.FullyQualifiedEntityName
+import org.apache.openwhisk.core.scheduler.SchedulingConfig
import scala.concurrent.{ExecutionContext, Future}
import scala.util.{Failure, Success}
class SchedulingDecisionMaker(
invocationNamespace: String,
action: FullyQualifiedEntityName,
- StaleThreshold: Double = 100.0)(implicit val actorSystem: ActorSystem, ec: ExecutionContext, logging: Logging)
+ schedulingConfig: SchedulingConfig)(implicit val actorSystem: ActorSystem, ec: ExecutionContext, logging: Logging)
Review Comment:
Would it make more sense to load the config once in the object and reference it from the class? The config shouldn't need to be loaded more than one time on a single run of the application
--
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
[GitHub] [openwhisk] bdoyle0182 commented on a diff in pull request #5232: Introduce scheduling configurations.
Posted by GitBox <gi...@apache.org>.
bdoyle0182 commented on code in PR #5232:
URL: https://github.com/apache/openwhisk/pull/5232#discussion_r869643275
##########
core/scheduler/src/main/resources/application.conf:
##########
@@ -64,6 +64,11 @@ whisk {
grpc {
tls = "false"
}
+ scheduling {
Review Comment:
since this is already under `scheduler` I think this isn't providing any more descriptiveness. How about calling the config `decision-maker`?
--
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
[GitHub] [openwhisk] ningyougang commented on a diff in pull request #5232: Introduce scheduling configurations.
Posted by GitBox <gi...@apache.org>.
ningyougang commented on code in PR #5232:
URL: https://github.com/apache/openwhisk/pull/5232#discussion_r869789279
##########
core/scheduler/src/main/resources/application.conf:
##########
@@ -64,6 +64,11 @@ whisk {
grpc {
tls = "false"
}
+ scheduling {
+ stale-threshold = "100 milliseconds"
Review Comment:
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)
##########
core/scheduler/src/main/resources/application.conf:
##########
@@ -64,6 +64,11 @@ whisk {
grpc {
tls = "false"
}
+ scheduling {
+ stale-threshold = "100 milliseconds"
Review Comment:
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)
--
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
[GitHub] [openwhisk] bdoyle0182 commented on a diff in pull request #5232: Introduce scheduling configurations.
Posted by GitBox <gi...@apache.org>.
bdoyle0182 commented on code in PR #5232:
URL: https://github.com/apache/openwhisk/pull/5232#discussion_r882016241
##########
core/scheduler/src/main/resources/application.conf:
##########
@@ -64,6 +64,11 @@ whisk {
grpc {
tls = "false"
}
+ scheduling {
+ stale-threshold = "100 milliseconds"
+ check-interval = "100 milliseconds"
+ drop-interval = "10 seconds"
Review Comment:
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
--
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
[GitHub] [openwhisk] style95 commented on a diff in pull request #5232: Introduce scheduling configurations.
Posted by GitBox <gi...@apache.org>.
style95 commented on code in PR #5232:
URL: https://github.com/apache/openwhisk/pull/5232#discussion_r869197606
##########
core/scheduler/src/main/scala/org/apache/openwhisk/core/scheduler/queue/SchedulingDecisionMaker.scala:
##########
@@ -3,16 +3,19 @@ package org.apache.openwhisk.core.scheduler.queue
import akka.actor.{Actor, ActorSystem, Props}
import org.apache.openwhisk.common.Logging
import org.apache.openwhisk.core.entity.FullyQualifiedEntityName
+import org.apache.openwhisk.core.scheduler.SchedulingConfig
import scala.concurrent.{ExecutionContext, Future}
import scala.util.{Failure, Success}
class SchedulingDecisionMaker(
invocationNamespace: String,
action: FullyQualifiedEntityName,
- StaleThreshold: Double = 100.0)(implicit val actorSystem: ActorSystem, ec: ExecutionContext, logging: Logging)
+ schedulingConfig: SchedulingConfig)(implicit val actorSystem: ActorSystem, ec: ExecutionContext, logging: Logging)
Review Comment:
Passing the entire config object looks overhead on the surface but we might add more scheduling configurations in the future.
It would make it easier to achieve.
##########
tests/src/test/scala/org/apache/openwhisk/core/invoker/test/DefaultInvokerServerTests.scala:
##########
@@ -83,9 +83,11 @@ class DefaultInvokerServerTests
val validCredentials = BasicHttpCredentials(systemUsername, systemPassword)
Get(s"/isEnabled") ~> addCredentials(validCredentials) ~> Route.seal(server.routes(tid)) ~> check {
status should be(OK)
- Unmarshal(responseEntity).to[String].map(response => {
- InvokerEnabled.parseJson(response) shouldEqual InvokerEnabled(true)
- })
+ Unmarshal(responseEntity)
Review Comment:
I am unclear why scalaFmt suddenly requires this kind of change.
##########
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,
Review Comment:
Seems this is no longer being used.
--
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
[GitHub] [openwhisk] style95 commented on a diff in pull request #5232: Introduce scheduling configurations.
Posted by GitBox <gi...@apache.org>.
style95 commented on code in PR #5232:
URL: https://github.com/apache/openwhisk/pull/5232#discussion_r869805091
##########
core/scheduler/src/main/resources/application.conf:
##########
@@ -64,6 +64,11 @@ whisk {
grpc {
tls = "false"
}
+ scheduling {
+ stale-threshold = "100 milliseconds"
Review Comment:
I just thought its value includes the unit so we can easily figure out its 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
[GitHub] [openwhisk] style95 commented on a diff in pull request #5232: Introduce scheduling configurations.
Posted by GitBox <gi...@apache.org>.
style95 commented on code in PR #5232:
URL: https://github.com/apache/openwhisk/pull/5232#discussion_r869802069
##########
core/scheduler/src/main/resources/application.conf:
##########
@@ -64,6 +64,11 @@ whisk {
grpc {
tls = "false"
}
+ scheduling {
+ stale-threshold = "100 milliseconds"
+ check-interval = "100 milliseconds"
+ drop-interval = "10 seconds"
Review Comment:
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
--
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
[GitHub] [openwhisk] style95 commented on a diff in pull request #5232: Introduce scheduling configurations.
Posted by GitBox <gi...@apache.org>.
style95 commented on code in PR #5232:
URL: https://github.com/apache/openwhisk/pull/5232#discussion_r869803906
##########
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
Review Comment:
The `FiniteDuration` of scala does not extend `Comparable`.
Since it compares duration based on `Duration` of java, it requires that.
--
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
[GitHub] [openwhisk] bdoyle0182 commented on a diff in pull request #5232: Introduce scheduling configurations.
Posted by GitBox <gi...@apache.org>.
bdoyle0182 commented on code in PR #5232:
URL: https://github.com/apache/openwhisk/pull/5232#discussion_r869645353
##########
ansible/group_vars/all:
##########
@@ -513,6 +513,10 @@ scheduler:
inProgressJobRetention: "{{ scheduler_inProgressJobRetention | default('20 seconds') }}"
managedFraction: "{{ scheduler_managed_fraction | default(1.0 - (scheduler_blackbox_fraction | default(__scheduler_blackbox_fraction))) }}"
blackboxFraction: "{{ scheduler_blackbox_fraction | default(__scheduler_blackbox_fraction) }}"
+ scheduling:
+ staleThreshold: "{{ scheduler_scheduling_staleThreshold | default('100 milliseconds') }}"
+ checkInterval: "{{ scheduler_scheduling_checkInterval | default('100 milliseconds') }}"
+ dropInterval: "{{ scheduler_scheduling_dropInterval | default('10 seconds') }}"
Review Comment:
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 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
[GitHub] [openwhisk] style95 commented on a diff in pull request #5232: Introduce scheduling configurations.
Posted by GitBox <gi...@apache.org>.
style95 commented on code in PR #5232:
URL: https://github.com/apache/openwhisk/pull/5232#discussion_r869805671
##########
core/scheduler/src/main/scala/org/apache/openwhisk/core/scheduler/queue/SchedulingDecisionMaker.scala:
##########
@@ -3,16 +3,19 @@ package org.apache.openwhisk.core.scheduler.queue
import akka.actor.{Actor, ActorSystem, Props}
import org.apache.openwhisk.common.Logging
import org.apache.openwhisk.core.entity.FullyQualifiedEntityName
+import org.apache.openwhisk.core.scheduler.SchedulingConfig
import scala.concurrent.{ExecutionContext, Future}
import scala.util.{Failure, Success}
class SchedulingDecisionMaker(
invocationNamespace: String,
action: FullyQualifiedEntityName,
- StaleThreshold: Double = 100.0)(implicit val actorSystem: ActorSystem, ec: ExecutionContext, logging: Logging)
+ schedulingConfig: SchedulingConfig)(implicit val actorSystem: ActorSystem, ec: ExecutionContext, logging: Logging)
Review Comment:
I wanted to pass the same object to both MemoryQueue and DecisionMaker and as you said it's easier to define unit tests.
--
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
[GitHub] [openwhisk] style95 commented on a diff in pull request #5232: Introduce scheduling configurations.
Posted by GitBox <gi...@apache.org>.
style95 commented on code in PR #5232:
URL: https://github.com/apache/openwhisk/pull/5232#discussion_r869804598
##########
core/scheduler/src/main/resources/application.conf:
##########
@@ -64,6 +64,11 @@ whisk {
grpc {
tls = "false"
}
+ scheduling {
Review Comment:
These configurations are related to both MemoryQueue and DecisionMaker.
The two intervals are not used by the decision-maker.
##########
ansible/group_vars/all:
##########
@@ -513,6 +513,10 @@ scheduler:
inProgressJobRetention: "{{ scheduler_inProgressJobRetention | default('20 seconds') }}"
managedFraction: "{{ scheduler_managed_fraction | default(1.0 - (scheduler_blackbox_fraction | default(__scheduler_blackbox_fraction))) }}"
blackboxFraction: "{{ scheduler_blackbox_fraction | default(__scheduler_blackbox_fraction) }}"
+ scheduling:
+ staleThreshold: "{{ scheduler_scheduling_staleThreshold | default('100 milliseconds') }}"
+ checkInterval: "{{ scheduler_scheduling_checkInterval | default('100 milliseconds') }}"
+ dropInterval: "{{ scheduler_scheduling_dropInterval | default('10 seconds') }}"
Review Comment:
This is just an interval to check to see if there are any stale activations.
--
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
[GitHub] [openwhisk] style95 commented on pull request #5232: Introduce scheduling configurations.
Posted by GitBox <gi...@apache.org>.
style95 commented on PR #5232:
URL: https://github.com/apache/openwhisk/pull/5232#issuecomment-1130938420
It's surprising that `scalaFmt` could not detect the error.
```bash
> 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.
--
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
[GitHub] [openwhisk] bdoyle0182 commented on a diff in pull request #5232: Introduce scheduling configurations.
Posted by GitBox <gi...@apache.org>.
bdoyle0182 commented on code in PR #5232:
URL: https://github.com/apache/openwhisk/pull/5232#discussion_r869508688
##########
core/scheduler/src/main/resources/application.conf:
##########
@@ -64,6 +64,11 @@ whisk {
grpc {
tls = "false"
}
+ scheduling {
+ stale-threshold = "100 milliseconds"
+ check-interval = "100 milliseconds"
+ drop-interval = "10 seconds"
Review Comment:
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
--
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
[GitHub] [openwhisk] bdoyle0182 commented on a diff in pull request #5232: Introduce scheduling configurations.
Posted by GitBox <gi...@apache.org>.
bdoyle0182 commented on code in PR #5232:
URL: https://github.com/apache/openwhisk/pull/5232#discussion_r869514340
##########
core/scheduler/src/main/scala/org/apache/openwhisk/core/scheduler/queue/SchedulingDecisionMaker.scala:
##########
@@ -3,16 +3,19 @@ package org.apache.openwhisk.core.scheduler.queue
import akka.actor.{Actor, ActorSystem, Props}
import org.apache.openwhisk.common.Logging
import org.apache.openwhisk.core.entity.FullyQualifiedEntityName
+import org.apache.openwhisk.core.scheduler.SchedulingConfig
import scala.concurrent.{ExecutionContext, Future}
import scala.util.{Failure, Success}
class SchedulingDecisionMaker(
invocationNamespace: String,
action: FullyQualifiedEntityName,
- StaleThreshold: Double = 100.0)(implicit val actorSystem: ActorSystem, ec: ExecutionContext, logging: Logging)
+ schedulingConfig: SchedulingConfig)(implicit val actorSystem: ActorSystem, ec: ExecutionContext, logging: Logging)
Review Comment:
Maybe it's valuable for the unit tests, I'll leave it up to you
--
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
[GitHub] [openwhisk] bdoyle0182 commented on a diff in pull request #5232: Introduce scheduling configurations.
Posted by GitBox <gi...@apache.org>.
bdoyle0182 commented on code in PR #5232:
URL: https://github.com/apache/openwhisk/pull/5232#discussion_r880847283
##########
core/scheduler/src/main/resources/application.conf:
##########
@@ -64,6 +64,11 @@ whisk {
grpc {
tls = "false"
}
+ scheduling {
+ stale-threshold = "100 milliseconds"
+ check-interval = "100 milliseconds"
+ drop-interval = "10 seconds"
Review Comment:
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
--
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
[GitHub] [openwhisk] bdoyle0182 commented on pull request #5232: Introduce scheduling configurations.
Posted by GitBox <gi...@apache.org>.
bdoyle0182 commented on PR #5232:
URL: https://github.com/apache/openwhisk/pull/5232#issuecomment-1130405003
LGTM
--
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
[GitHub] [openwhisk] bdoyle0182 commented on pull request #5232: Introduce scheduling configurations.
Posted by GitBox <gi...@apache.org>.
bdoyle0182 commented on PR #5232:
URL: https://github.com/apache/openwhisk/pull/5232#issuecomment-1142480729
Looks like the build passed, I'm good with merging in
--
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
[GitHub] [openwhisk] style95 commented on pull request #5232: Introduce scheduling configurations.
Posted by GitBox <gi...@apache.org>.
style95 commented on PR #5232:
URL: https://github.com/apache/openwhisk/pull/5232#issuecomment-1140565798
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)
```
--
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
[GitHub] [openwhisk] ningyougang commented on a diff in pull request #5232: Introduce scheduling configurations.
Posted by GitBox <gi...@apache.org>.
ningyougang commented on code in PR #5232:
URL: https://github.com/apache/openwhisk/pull/5232#discussion_r869789279
##########
core/scheduler/src/main/resources/application.conf:
##########
@@ -64,6 +64,11 @@ whisk {
grpc {
tls = "false"
}
+ scheduling {
+ stale-threshold = "100 milliseconds"
Review Comment:
Because the value is `FiniteDuration` type, how about `stale-duration-threshold`?
(btw, `check-interval or drop-interval`, from the name, we can know it is a `FiniteDuration` 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
[GitHub] [openwhisk] jiangpengcheng commented on pull request #5232: Introduce scheduling configurations.
Posted by GitBox <gi...@apache.org>.
jiangpengcheng commented on PR #5232:
URL: https://github.com/apache/openwhisk/pull/5232#issuecomment-1130863291
looks like there is a compile error
--
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
[GitHub] [openwhisk] style95 commented on a diff in pull request #5232: Introduce scheduling configurations.
Posted by GitBox <gi...@apache.org>.
style95 commented on code in PR #5232:
URL: https://github.com/apache/openwhisk/pull/5232#discussion_r884429165
##########
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"
Review Comment:
I reverted this change as it introduces a compatibility issue.
--
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
[GitHub] [openwhisk] style95 merged pull request #5232: Introduce scheduling configurations.
Posted by GitBox <gi...@apache.org>.
style95 merged PR #5232:
URL: https://github.com/apache/openwhisk/pull/5232
--
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
[GitHub] [openwhisk] bdoyle0182 commented on pull request #5232: Introduce scheduling configurations.
Posted by GitBox <gi...@apache.org>.
bdoyle0182 commented on PR #5232:
URL: https://github.com/apache/openwhisk/pull/5232#issuecomment-1139831174
Is it just the build blocking this right now?
--
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
[GitHub] [openwhisk] style95 commented on pull request #5232: Introduce scheduling configurations.
Posted by GitBox <gi...@apache.org>.
style95 commented on PR #5232:
URL: https://github.com/apache/openwhisk/pull/5232#issuecomment-1140397706
> Is it just the build blocking this right now?
I think so.
--
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