[PR-4]Decision making module for automatic migration by AmitRoushan · Pull Request #793 · amino-os/Amino.Run

AmitRoushan · 2019-06-04T06:31:53Z

PR has following changes:
class for periodically watch metric and call prediction module for top migration candidate
class for predict kernel server node for replica.

Note:

PR is written on top of [PR-2]Microservice level metrics[data IN/OUT, RPC processing time] measurment and collection at DM's server policy maintained per client #746 and [PR-3]Storing Kernel Server Metrics and Microservice Metrics on OMS and provide get metric APIs to decision making module #753. [PR-2]Microservice level metrics[data IN/OUT, RPC processing time] measurment and collection at DM's server policy maintained per client #746 and [PR-3]Storing Kernel Server Metrics and Microservice Metrics on OMS and provide get metric APIs to decision making module #753 are closed and [PR-1]Node level metrics[latency and data rate] measurement on Kernel Servers #794, [PR-2]Microservice level metrics[data IN/OUT, RPC processing time] measurment and collection at DM's server policy maintained per client #795, [PR-3]Storing Kernel Server Metrics and Microservice Metrics on OMS and provide get metric APIs to decision making module #796 are opened for handling metric management.
This PR has addressed review comments for [WIP] Decision making module for automatic migration #745

quinton-hoole · 2019-06-05T02:27:47Z

Thanks. Please rebase before requesting a review.

quinton-hoole · 2019-06-07T23:51:25Z

Please check that the comments describing this PR are correct (all the PR's mentioned there have been closed, so I assume it's out of date).
And please confirm that all of this PR needs to be reviewed? It's quite huge.
Then please self-review.
Then ask @VenuReddy2103 to review.
Then ask me to do a final review before merging.
Thanks.

AmitRoushan · 2019-06-14T05:28:03Z

Review comment fix from PR #745:

General convention for package name is to use lowercase
Status: "migrationDecision" module changed to "migrationdecision".
Don't need logger for each instance of it. Make it private static final.
Status: MetricWatcher logger changed to "private static final"
Why transient volatile ?
Status: Removed "transient" modifier from ResettableTimer in MetricWatcher
Instead if timer != null, then simply return can be more clear ?
Status: Timer instance creation moved to constructor. So removed null check from start method of MetricWatcher
What we need was nodeMetric from client to the kernelserver. But we took from kernelserver to the client. Did we assume both to be same ? They both are not necessary to be same.
Status: Updated code as per suggestion. Now rate and latency are used from kernelClient node metric.

AmitRoushan · 2019-06-14T06:02:30Z

@VenuReddy2103 , I have handled all review comments from PR #745. Please have a look.
@quinton-hoole #746 and #753 are closed and new PR #794, #795 and #796 are open respectively for microservice and node metric management. Same has been updated in PR description.

quinton-hoole · 2019-06-17T22:20:38Z

@AmitRoushan I still cannot review this PR until #794 , #795 and #796 have been reviewed. And I can't review those until #794 (comment) has been answered. So basically all of these PR's are blocked.

AmitRoushan · 2019-09-06T15:01:42Z

@quinton-hoole
I have reverted KVstore code changes. Changes are made to test migration.
Also we fixed one bug , found during testing.
Please find the attached status report below:

Build report
KVStore App report
Hankstodo App report
MinnieTwitter App report
HankstodoRuby App report
KVstorejs App report

quinton-hoole · 2019-09-06T17:31:24Z


            // Register against OMS
            ServerInfo srvInfo = createServerInfo(host, ksArgs.labels);
+            srvInfo.processorCount = Runtime.getRuntime().availableProcessors();


It's not clear to me why this initialization is not done in createServerInfo like all the other initialization?
Or better still, in the constructor of ServerInfo?
I'm uncomfortable with having an uninitialized processorCount in ServerInfo structure until someone remembers to initialize it.

quinton-hoole · 2019-09-06T21:08:47Z

+        } catch (Exception e) {
+            logger.warning("Failed to get replica dispatcher");
+            e.printStackTrace();
+            return false;


It seems it would be more appropriate to throw an exception here.
What does the return value of this method actually mean?

quinton-hoole · 2019-09-07T00:03:14Z

+            } catch (MicroServiceNotFoundException e) {
+                logger.warning(
+                        String.format(
+                                "Skipping Replica [%s] evaluation as micro service not available",


Lets improve this exception message. It's not the microservice that's unavailable. It's not found in OMS.

quinton-hoole · 2019-09-07T00:03:35Z

+            } catch (MicroServiceReplicaNotFoundException e) {
+                logger.warning(
+                        String.format(
+                                "Skipping Replica [%s] evaluation as micro service replica not available",


quinton-hoole · 2019-09-07T00:09:50Z

+                    KernelServerNotFoundException {
+        /*
+           To measure time consumption by vertex Vk we use the following formula
+           tk = Tk,posk+i + summation { (Ski+Sik)/Rposk,posi + Lposk,posi }


Please explain what all these terms mean. tK, Tk, posk+i, Ski, Sik, Rposk? It's not at all clear what you mean here.

quinton-hoole · 2019-09-07T00:12:29Z

+                serverManager.getKernelServerMetric(replicaKernelServer);
+
+        // get metrics stat of RPC processing time from different kernel client
+        // TODO currently considering processing time from different kernel client for simplicity.


Please clarify. It's not clear what you mean here.

quinton-hoole · 2019-09-07T00:13:54Z

+        // calculate time factor impact on RPC with latency and bandwidth
+        long averageExecutionTime = 0;
+        long summation = 0;
+        long data;


Please call this dataSize. It's not data.

quinton-hoole · 2019-09-07T00:18:20Z

+
+        // calculate time factor impact on RPC with latency and bandwidth
+        long averageExecutionTime = 0;
+        long summation = 0;


This is not a useful variable name. Do you mean the sum of the round trip times? Perhaps call it rttSum?

quinton-hoole · 2019-09-07T00:53:23Z

+        }
+
+        AbstractMap.SimpleEntry<InetSocketAddress, Long> bestTimeOptimization =
+                predictedTime.first();


I think it would be worth mentioning in a comment that you're relying on this set being sorted by time here. Sets are not usually sorted, so this is an unusual use of a set, and a bit confusing.

quinton-hoole · 2019-09-07T01:01:10Z

+            replicaMetrics = microServiceManager.getMicroServiceMetric(replicaID);
+        } catch (MicroServiceNotFoundException e) {
+            e.printStackTrace();
+            return 0;


This exception should be thrown. This also answers my previous question above.

quinton-hoole · 2019-09-07T01:02:57Z

+            currentKernelServerMetrics = serverManager.getKernelServerMetric(currentKernelServer);
+            futureKernelServerMetrics = serverManager.getKernelServerMetric(futureKernelServer);
+        } catch (KernelServerNotFoundException e) {
+            e.printStackTrace();


Throw this exception.

quinton-hoole · 2019-09-07T01:04:24Z

+        return bestTimeOptimization.getKey();
+    }
+
+    private long getOptimizedTime(


Add a comment to explain what this method does.

quinton-hoole · 2019-09-07T01:15:04Z

+        return bestTimeOptimization.getKey();
+    }
+
+    private long getOptimizedTime(


nit: It's not actually an optimization, it's a prediction of elapsed time on a different node.

quinton-hoole · 2019-09-07T01:16:32Z

+    private long getOptimizedTime(
+            ReplicaID replicaID,
+            InetSocketAddress currentKernelServer,
+            InetSocketAddress futureKernelServer)


nit: It's not actually a future kernel server. It's a prediction related to an potential alternative kernel server.

quinton-hoole · 2019-09-07T01:18:01Z

+                                kernelClient,
+                                futureKernelServer,
+                                currentByteExchanged);
+            } catch (Exception e) {


We can't just catch Exceptions and ignore them like this.

quinton-hoole · 2019-09-07T01:22:06Z

+        long currentProcessingTime;
+        long currentByteExchanged;
+        long futureProcessingTime;
+        double currentIOTime;


Lets avoid all double precision arithmetic. It's unnecessary. Just record times in a small enough unit (e.g. microseconds or nanoseconds) and just use integer arithmetic.

quinton-hoole · 2019-09-07T01:23:53Z

+                            "Node metric not available between kernel client [%s] and kernel server [%s]",
+                            kernelClient, KernelServer);
+            logger.warning(msg);
+            throw new Exception(msg);


Please throw a more specific exception type. This is a not found exception.

quinton-hoole · 2019-09-07T01:26:39Z

                /* Store these metrics to make them available for statistics and decision making module */
                updateMetric(object.replicaId, object.metrics);
                return;
+            } else if (notification instanceof MigrationNotification) {


I still don't understand why we're using notifications for all of these things.
Surely we should just have a method to receive metrics, and another one to receive migration instructions?
What's the benefit of using Notifications instead?

quinton-hoole

See inline comments for requested changes. Amit, if you are otherwise occupied, I can make the required changes. Let me know.

AmitRoushan · 2019-09-09T11:56:59Z

@quinton-hoole
I have fixed most of review comments but still i need some more time to run some test scenario for migration.

quinton-hoole · 2019-09-09T14:43:42Z

Thanks Amit

…

On Mon, Sep 9, 2019, 04:57 Amit Roushan ***@***.***> wrote: @quinton-hoole <https://github.com/quinton-hoole> I have fixed most of review comments but still i need some more time to run some test scenario for migration. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#793?email_source=notifications&email_token=AKNAA6H25X3YXZABUOV67MLQIY2Y3A5CNFSM4HSZF2U2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD6HJGPY#issuecomment-529437503>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AKNAA6AQPSFU42J76OXHWHDQIY2Y3ANCNFSM4HSZF2UQ> .

quinton-hoole · 2019-09-09T21:27:14Z

+
    public ServerInfo(InetSocketAddress addr) {
        this.host = addr;
+        this.processorCount = Runtime.getRuntime().availableProcessors();


This will only work correctly when this constructor is called on the machine to which the server info refers. Surely processorCount should be a parameter to the constructor, similar to addr?

quinton-hoole · 2019-09-09T21:30:29Z

-    private TreeSet<AbstractMap.SimpleEntry<ReplicaID, Long>> replicaHeap;
+    private TreeSet<AbstractMap.SimpleEntry<ReplicaID, Long>>
+            replicaHeap; // should only used in getBestCandidates for selecting best microservice
+    // replica for migration


Nit - weird comment formatting. Use /* */ instead?

quinton-hoole · 2019-09-09T21:32:38Z

+                                                String.format(
+                                                        "Skipping Replica [%s] for migration as micro service not found",
+                                                        bestCandidateReplicaId));
+                                        e.printStackTrace();


Why do you need to print the stack trace?

quinton-hoole · 2019-09-09T21:33:07Z

+                                                String.format(
+                                                        "Skipping Replica [%s] for migration as micro service replica not found",
+                                                        bestCandidateReplicaId));
+                                        e.printStackTrace();


quinton-hoole · 2019-09-09T23:34:24Z

        }

-        return averageExecutionTime + summation;
+        return avgElapsedTime + avgRtt;


This seems wrong. Don't you want to subtract the RTT from the elapsedTime to get the processing (computation) time? Also, a single inbound RPC may result in multiple "dowstream" outbound RPC's, some in serial, and some in parallel. Using the avgRtt here (even assuming your substract it as mentioned above) implies a single outbound RPC per inbound RPC, which is not correct? I think you need to keep track of the ratio of inbound:outbound (say 1:n) and subtract n times the avgRtt?

quinton-hoole · 2019-09-13T19:23:54Z

@AmitRoushan How is your testing going?

quinton-hoole · 2019-09-17T20:49:36Z

@AmitRoushan ?

quinton-hoole · 2019-10-04T20:53:09Z

@AmitRoushan @VenuReddy2103 What is the status with this PR please?

quinton-hoole · 2019-11-05T17:49:27Z

@AmitRoushan @VenuReddy2103 What is the status with this PR please?

AmitRoushan mentioned this pull request Jun 4, 2019

[WIP] Decision making module for automatic migration #745

Closed

AmitRoushan force-pushed the metricCollector branch 2 times, most recently from 844fc98 to 6a10160 Compare June 6, 2019 14:19

quinton-hoole assigned VenuReddy2103 Jun 26, 2019

quinton-hoole mentioned this pull request Jun 28, 2019

Current logging enhancement #810

Open

AmitRoushan changed the title ~~Decision making module for automatic migration~~ [PR-4]Decision making module for automatic migration Jul 25, 2019

AmitRoushan force-pushed the metricCollector branch from 524c65f to 9d17cca Compare July 25, 2019 06:42

quinton-hoole mentioned this pull request Sep 6, 2019

Rebase pr4 decision making module #815

Closed

AmitRoushan added 3 commits September 6, 2019 15:56

added migration decision making module

1fdd68f

review comment fix

3a8996e

rebased with latest code

da3d2e8

AmitRoushan force-pushed the metricCollector branch 3 times, most recently from 4ffa543 to 9b5a943 Compare September 6, 2019 14:39

corrected kvstore app

74c7010

AmitRoushan force-pushed the metricCollector branch from 9b5a943 to 74c7010 Compare September 6, 2019 14:45

quinton-hoole reviewed Sep 6, 2019

View reviewed changes

quinton-hoole reviewed Sep 7, 2019

View reviewed changes

quinton-hoole requested changes Sep 7, 2019

View reviewed changes

review comment fixes

4fbea74

AmitRoushan force-pushed the metricCollector branch from 7ed595b to 4fbea74 Compare September 9, 2019 11:54

quinton-hoole reviewed Sep 9, 2019

View reviewed changes

AmitRoushan force-pushed the metricCollector branch 4 times, most recently from 6ce13b0 to 45045cf Compare September 18, 2019 12:57

added some additional comments

abf7bdc

AmitRoushan force-pushed the metricCollector branch from 45045cf to abf7bdc Compare September 18, 2019 13:35

Uh oh!

Conversation

AmitRoushan commented Jun 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

quinton-hoole commented Jun 5, 2019

Uh oh!

quinton-hoole commented Jun 7, 2019

Uh oh!

AmitRoushan commented Jun 14, 2019

Uh oh!

AmitRoushan commented Jun 14, 2019

Uh oh!

quinton-hoole commented Jun 17, 2019

Uh oh!

AmitRoushan commented Sep 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

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!

quinton-hoole left a comment

Choose a reason for hiding this comment

Uh oh!

AmitRoushan commented Sep 9, 2019

Uh oh!

quinton-hoole commented Sep 9, 2019 via email

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!

quinton-hoole commented Sep 13, 2019

Uh oh!

quinton-hoole commented Sep 17, 2019

Uh oh!

quinton-hoole commented Oct 4, 2019

Uh oh!

quinton-hoole commented Nov 5, 2019

Uh oh!

Reviewers

AmitRoushan commented Jun 4, 2019 •

edited

Loading

AmitRoushan commented Sep 6, 2019 •

edited

Loading