Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

support for rpc polling #504

Merged
merged 16 commits into from Mar 3, 2016
Merged

support for rpc polling #504

merged 16 commits into from Mar 3, 2016

Conversation

debris
Copy link
Collaborator

@debris debris commented Feb 23, 2016

fixes #551

@debris debris added the A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. label Feb 23, 2016
tomusdrw
tomusdrw reviewed Feb 23, 2016
View reviewed changes
ethcore/src/client.rs
blocks.sort();

blocks.into_iter()
.map(|number| self.chain.read().unwrap().block_hash(number).map(|hash| (number, hash)))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lock could be extracted?

@gavofyork
Copy link
Contributor

this has 3,632 + 562 lines altered. that's way too much. i know that a lot of these lines are moving stuff around and some will be from #418, but assuming that #418 doesn't account for > 4k of those lines, the refactoring should be in a separate PR. please rearrange into multiple PRs that are either large but utterly trivial, or small (< 500 total lines changed) with new logic.

@gavofyork gavofyork added A4-gotissues 💥 Pull request is reviewed and has significant issues which must be addressed. A3-justtoobig and removed A4-gotissues 💥 Pull request is reviewed and has significant issues which must be addressed. labels Feb 26, 2016
@debris
Copy link
Collaborator Author

debris commented Feb 26, 2016

it actually has only 300 lines changed... don't know why github was displaying so many changes...

@gavofyork
Copy link
Contributor

fair enough. don't know what was going on before...

@arkpar
Copy link
Collaborator

arkpar commented Feb 27, 2016

needs a merge

@debris debris added A0-pleasereview 🤓 Pull request needs code review. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Mar 2, 2016
gavofyork
gavofyork reviewed Mar 2, 2016
View reviewed changes
rpc/src/v1/helpers/poll_manager.rs
pub fn new_with_timer(timer: T) -> Self {
PollManager {
polls: TransientHashMap::new_with_timer(POLL_LIFETIME, timer),
next_available_id: 0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

trailing , missing

@gavofyork gavofyork added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Mar 2, 2016
@gavofyork
Copy link
Contributor

a few minor issues/questions. would be nice to see comprehensive tests for rpc/src/v1/impls/eth.rs as it's getting quite logic-heavy now (yeah, i know i'm contributing to that, too :-))

@debris
Copy link
Collaborator Author

debris commented Mar 2, 2016

issues fixed :) I will make rpc tests my priority now, cause they are the only dangling rpc task besides account management.

@debris debris added A0-pleasereview 🤓 Pull request needs code review. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Mar 2, 2016
@gavofyork gavofyork mentioned this pull request Mar 2, 2016
Closed
gavofyork pushed a commit that referenced this pull request Mar 3, 2016
Merge pull request #504 from ethcore/rpc_poll_ids
09e01fa 
support for rpc polling
@gavofyork gavofyork merged commit 09e01fa into master Mar 3, 2016
@gavofyork gavofyork deleted the rpc_poll_ids branch March 3, 2016 11:44
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
A0-pleasereview 🤓 Pull request needs code review.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

JSONRPC should manage multiple connections at once
4 participants
@debris @gavofyork @arkpar @tomusdrw