search

amino-os / Amino.Run

Amino Distributed OS - Runtime Manager
Apache License 2.0
29 stars 12 forks source link

[PR-4]Decision making module for automatic migration #793

Open AmitRoushan opened 5 years ago

AmitRoushan commented 5 years ago

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:

  1. PR is written on top of #746 and #753. #746 and #753 are closed and #794, #795, #796 are opened for handling metric management.
  2. This PR has addressed review comments for #745
quinton-hoole commented 5 years ago

Thanks. Please rebase before requesting a review.

quinton-hoole commented 5 years ago

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 commented 5 years ago

Review comment fix from PR #745:

  1. General convention for package name is to use lowercase Status: "migrationDecision" module changed to "migrationdecision".
  2. Don't need logger for each instance of it. Make it private static final. Status: MetricWatcher logger changed to "private static final"
  3. Why transient volatile ? Status: Removed "transient" modifier from ResettableTimer in MetricWatcher
  4. 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
  5. 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 commented 5 years ago

@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 commented 5 years ago

@AmitRoushan I still cannot review this PR until #794 , #795 and #796 have been reviewed. And I can't review those until https://github.com/amino-os/Amino.Run/pull/794#issuecomment-500074934 has been answered. So basically all of these PR's are blocked.

AmitRoushan commented 4 years ago

@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:

AmitRoushan commented 4 years ago

@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 commented 4 years ago

Thanks Amit

On Mon, Sep 9, 2019, 04:57 Amit Roushan notifications@github.com 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 https://github.com/amino-os/Amino.Run/pull/793?email_source=notifications&email_token=AKNAA6H25X3YXZABUOV67MLQIY2Y3A5CNFSM4HSZF2U2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD6HJGPY#issuecomment-529437503, or mute the thread https://github.com/notifications/unsubscribe-auth/AKNAA6AQPSFU42J76OXHWHDQIY2Y3ANCNFSM4HSZF2UQ .

quinton-hoole commented 4 years ago

@AmitRoushan How is your testing going?

quinton-hoole commented 4 years ago

@AmitRoushan ?

quinton-hoole commented 4 years ago

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

quinton-hoole commented 4 years ago

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