-
Notifications
You must be signed in to change notification settings - Fork 20.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
les: handler separation #19639
les: handler separation #19639
Conversation
4bbc4ac
to
e5ff2cd
Compare
f02b6ab
to
b79d16b
Compare
b79d16b
to
8b05adb
Compare
8b05adb
to
b598746
Compare
@zsfelfoldi Could you please take a look when you back :) This PR basically separates the client handler and server handler into two structures so that it's easier to read. Besides in the server handler, this PR replaces some direct disk access with cached enabled approach( |
389e104
to
3aab5a3
Compare
3aab5a3
to
eb55adf
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most of the PR looks good. It is hard to judge the correctness of such a huge refactoring though. Please take another look at the server handler (accept and sendResponse fns in particular) because there are some changes that I am not sure about. If it is just out of sync with latest master then please fix it, otherwise comment why those changes have been made. It is a really sensitive part of the code.
9bd903c
to
f72f115
Compare
@zsfelfoldi I address the comments. I know it's super hard to review such a huge refactor PR. I review it a few times and do it again today. I think it should sync with master branch. But really hope you can check it again to ensure this PR doesn't break something :)) Btw there is a tiny conflict with master branch(it's only 2 line change I made a few days ago). I would leave it now. Since I always squash all commits and then rebase. I think the commit I made today probably will make it easier for you to catch all changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with some minor things to fix
core: address comments les: address comments les: fix metrics
3f1a4f7
to
77ae572
Compare
@zsfelfoldi Fixed and rebased. The fix is in the last commit. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The core parts looks good to me
4d35807
to
ec616b5
Compare
ec616b5
to
1865e5f
Compare
This PR refactors light client handler a bit by separating the client handler and server handler to two structures.