Able to Disable Worker #688
Conversation
joowon-byun
requested review from
ehnuje,
KimKyungup,
jeongkyun-oh,
aidan-kwon and
yoomee1313
| // set worker | ||
| if config.WorkerDisable { | ||
| cn.miner = work.NewFakeWorker() | ||
| } else { |
There was a problem hiding this comment.
Disable daemon when WorkerDisable flag is given?
There was a problem hiding this comment.
Not only daemon, whole statements in else should be pulled out.
Please ignore this comment.
|
Please run local test before pushing PR to github repo. |
| cmd.Run() | ||
| // set worker | ||
| if config.WorkerDisable { | ||
| cn.miner = work.NewFakeWorker() |
There was a problem hiding this comment.
What is difference between the woker and fake woker for EN, KES, PN ?
There was a problem hiding this comment.
If we use fake worker, istanbul backend is set but not started. If this happens, some of the validator functions are not working correctly, no handling a new round and so on.
EN, KES and PN needs worker to run.
There was a problem hiding this comment.
Please leave a TODO comment that we should implement auto restart feature when the fake worker is used.
There was a problem hiding this comment.
@ehnuje auto-restart is a little tricky. Auto-restart feature was for CNs, but CNs should not use the fake worker.
There was a problem hiding this comment.
@aidan-kwon Right. When I discussed this with @winnie-byun, we decided that auto-restart does not need to be implemented in the Klaytn code. This can be done by checking the liveness of the pod.
|
Dear reviewers, I found an error while testing. I will inform you when I resolve it. |
|
Dear reviewers, Disabling worker results in block processing failure. APIs regrading to proposers and validators are loaded from headers. I will inform you again after testing. |
Please share the failure case log or reason? |
|
@KimKyungup The fail case occured due to block processing failure. Dear reviewers, talking to @aidan-kwon, we've decided to merge this PR before testing PR#698 for conveince. |
| cmd.Run() | ||
| // set worker | ||
| if config.WorkerDisable { | ||
| cn.miner = work.NewFakeWorker() |
There was a problem hiding this comment.
Please leave a TODO comment that we should implement auto restart feature when the fake worker is used.
|
@KimKyungup @jeongkyun-oh @yoomee1313 PTAL :) |
Proposed changes
FakeWorkerworker.disableTypes of changes
Please put an x in the boxes related to your change.
Checklist
Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.
$ make test)Related issues
Further comments