[New Scheduler] Add DataManagementService by style95 · Pull Request #5063 · apache/openwhisk

style95 · 2021-02-10T04:10:03Z

Description

This component is in charge of storing data to ETCD.
It is based on eventual consistency.
If it fails to store data for some reason, it keeps retrying until data is stored.

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.

style95 · 2021-02-10T04:11:14Z

+case object GrantLease
+
+// TBD
+class LeaseKeepAliveService {


I realized this pr is also dependent on these modules.
It would be better to merge this PR after this kind of module is introduced.

bdoyle0182 · 2021-02-10T22:53:44Z

+    // normally these messages will be sent when queues are created.
+    case request: ElectLeader =>
+      if (inProgressKeys.contains(request.key)) {
+        logging.info(this, s"save request $request into a buffer")


do these really need to be info level?

In this case, it stores the request into a buffer because there is already precedent request processing. If any issue happens it would let us know if the request has processed or not.

So I'm still learning what this is doing, but what does each request involve? Is it every activation or some sort of metadata setup? If it's every activation it would seem spammy to me otherwise I think it's fine

bdoyle0182 · 2021-02-10T23:11:44Z

+            .map { res =>
+              parent ! FinishWork(request.key)
+              if (res.getSucceeded) {
+                logging.debug(this, s"data is stored.")


what data is stored?

style95 · 2021-02-11T01:51:41Z

I think we need to merge a PR for CI tests for scheduler components before merging this PR.
Since some downstream is running their CI tests based on the upstream core repo, we need to differentiate scheduler tests from the others. Need to see if we can setup another dedicated pipeline for scheduler components in Travis.
And when all components contribution is over, we can merge the pipeline into existing unit/system pipelines.

style95 · 2021-02-11T01:52:41Z

+ * In the event any issue occurs while storing data, the actor keeps trying until the data is stored guaranteeing delivery to ETCD.
+ * So it guarantees the data is eventually stored.
+ */
+class DataManagementService(watcherService: ActorRef, workerFactory: ActorRefFactory => ActorRef)(


I will include test cases into this PR after setting up the CI pipeline for scheduler components.

style95 · 2021-02-11T01:54:28Z

+      }
+
+    // normally these messages will be sent when queues are created.
+    case request: ElectLeader =>


Leader election happens when a queue is created.
This is to guarantee only one scheduler creates a certain queue.
So it happens relatively fewer times.

style95 · 2021-02-11T01:55:10Z

+
+    // normally these messages will be sent when queues are created.
+    case request: ElectLeader =>
+      if (inProgressKeys.contains(request.key)) {


With the retry nature of this component, if there is a precedent request(being retried), it would store the new request to a buffer.

style95 · 2021-02-11T01:55:42Z

+        logging.info(this, s"save a request $request into a buffer")
+        operations.getOrElseUpdate(request.key, Queue.empty[Any]).enqueue(request)
+      } else {
+        worker ! request


Actual works would be delegated to ETCDWorker.

style95 · 2021-02-11T01:56:45Z

+        inProgressKeys = inProgressKeys + request.key
+      }
+
+    case request: RegisterInitialData =>


Actions under the same namespace share some data such as namespace throttling data.
So it is required to store the data if there is no data yet but not overwrite an existing one.
This case is for the case.

style95 · 2021-02-11T01:56:59Z

+
+    case request: RegisterInitialData =>
+      // send WatchEndpoint first as the put operation will be retried until success if failed
+      if (request.failoverEnabled)


If the failover is enabled, it would watch the key and if the key is deleted for some reason, it would try to restore it.

style95 · 2021-02-11T01:58:17Z

+        inProgressKeys = inProgressKeys + request.key
+      }
+
+    case request: RegisterData =>


This will overwrite the existing data in ETCD.
Generally, this is used for data that is not shared among actions.

style95 · 2021-02-11T01:59:34Z

+    case WatchEndpointRemoved(_, key, value, true) =>
+      logging.error(this, s"unexpected data received: ${WatchEndpoint(key, value, isPrefix = true, watcherName)}")
+
+    case msg: UpdateDataOnChange =>


To reduce the loads against ETCD, it does not store data if there is no change in the value.

style95 · 2021-02-11T02:04:46Z

+  actorSystem: ActorSystem)
+    extends Actor {
+  private implicit val ec = context.dispatcher
+


This class is used by both schedulers and invokers to store data to ETCD.
The following kinds of data are stored to ETCD.

Throttling data(Action / Namespace)

Queue endpoint(where a queue is running)

Scheduler endpoint.

Container data(running container, warmed container, data to describe how many containers are being created)

Dependent modules are Queue, ContainerProxy, CreationJobManager, etc.

style95 · 2021-02-11T02:06:07Z

Just comes up in my mind is it would be great to write down some documents for each component in Wiki.

style95 · 2021-03-08T08:32:25Z

Waiting for this PR(#5067) to be merged.

style95 · 2021-03-08T09:10:31Z

I wrote a document about this module: https://cwiki.apache.org/confluence/display/OPENWHISK/DataManagementService

…DataManagementService.scala Apply comment Co-authored-by: Brendan Doyle <[email protected]>

Update comments Co-authored-by: Brendan Doyle <[email protected]>

codecov-io · 2021-03-11T09:11:04Z

Codecov Report

Merging #5063 (ec5f93f) into master (e05aa44) will decrease coverage by 6.59%.
The diff coverage is 44.69%.

@@            Coverage Diff             @@
##           master    #5063      +/-   ##
==========================================
- Coverage   81.63%   75.03%   -6.60%     
==========================================
  Files         205      214       +9     
  Lines       10013    10448     +435     
  Branches      442      470      +28     
==========================================
- Hits         8174     7840     -334     
- Misses       1839     2608     +769

Impacted Files	Coverage Δ
...la/org/apache/openwhisk/core/etcd/EtcdClient.scala	`26.22% <ø> (ø)`
...apache/openwhisk/core/service/WatcherService.scala	`91.66% <ø> (+2.08%)`	⬆️
...openwhisk/core/service/DataManagementService.scala	`44.27% <44.27%> (ø)`
.../scala/org/apache/openwhisk/core/WhiskConfig.scala	`95.48% <100.00%> (+0.02%)`	⬆️
...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%> (-95.85%)`	⬇️
...sk/core/database/cosmosdb/CosmosDBViewMapper.scala	`0.00% <0.00%> (-93.90%)`	⬇️
...tabase/cosmosdb/cache/CacheInvalidatorConfig.scala	`0.00% <0.00%> (-92.31%)`	⬇️
... and 26 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 e05aa44...ec5f93f. Read the comment docs.

style95 · 2021-03-15T06:16:59Z

It's ready to merge.

style95 · 2021-03-18T04:56:04Z

@bdoyle0182 Do you have any other comments on this PR?

bdoyle0182 · 2021-03-18T17:16:24Z

Just one comment. LGTM

style95 added the scheduler label Feb 10, 2021

style95 commented Feb 10, 2021

View reviewed changes

bdoyle0182 reviewed Feb 10, 2021

View reviewed changes

style95 changed the title ~~[New Scheduler] Add DataManagementService~~ [WIP][New Scheduler] Add DataManagementService Feb 11, 2021

style95 commented Feb 11, 2021

View reviewed changes

style95 force-pushed the add-data-mgmt-service branch from bab5ce6 to fd4d4e3 Compare February 18, 2021 09:05

ningyougang mentioned this pull request Mar 3, 2021

[New Scheduler] Implement InvokerHealthyManager #5061

Merged

21 tasks

style95 force-pushed the add-data-mgmt-service branch from a20e5d2 to 448753b Compare March 8, 2021 08:31

style95 and others added 7 commits March 11, 2021 10:36

Add DataManagementService

31e25a0

Update common/scala/src/main/scala/org/apache/openwhisk/core/service/…

0d24039

…DataManagementService.scala Apply comment Co-authored-by: Brendan Doyle <[email protected]>

Apply suggestions from code review

b5fad68

Update comments Co-authored-by: Brendan Doyle <[email protected]>

Apply comments

75e200a

Add unit tests for DataManagementService

30f62dc

Remove unused variable

6a18eb8

Add the license header

56a4f2c

style95 force-pushed the add-data-mgmt-service branch from 448753b to 56a4f2c Compare March 11, 2021 01:36

style95 changed the title ~~[WIP][New Scheduler] Add DataManagementService~~ [New Scheduler] Add DataManagementService Mar 11, 2021

Change Lease

c7ad646

Pull docker image for the api gateway in advance

ec5f93f

ningyougang approved these changes Mar 17, 2021

View reviewed changes

style95 merged commit ecb1509 into apache:master Mar 18, 2021

Uh oh!

Conversation

style95 commented Feb 10, 2021

Description

Related issue and scope

My changes affect the following components

Types of changes

Checklist:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

style95 commented Feb 11, 2021

Uh oh!

style95 Feb 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

style95 commented Feb 11, 2021

Uh oh!

style95 commented Mar 8, 2021

Uh oh!

style95 commented Mar 8, 2021

Uh oh!

codecov-io commented Mar 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

style95 commented Mar 15, 2021

Uh oh!

style95 commented Mar 18, 2021

Uh oh!

bdoyle0182 commented Mar 18, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

style95 Feb 11, 2021 •

edited

Loading

codecov-io commented Mar 11, 2021 •

edited

Loading