Skip to content
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

[ADR] [DRAFT] pubsub 2.0 #2532

Merged
merged 3 commits into from Nov 2, 2018
Merged

[ADR] [DRAFT] pubsub 2.0 #2532

merged 3 commits into from Nov 2, 2018

Conversation

melekes
Copy link
Contributor

@melekes melekes commented Oct 2, 2018

Refs #951, #1879, #1880

@melekes melekes requested a review from zramsay as a code owner October 2, 2018 16:40
@melekes melekes changed the title [WIP] [ADR] pubsub 2.0 [ADR] [DRAFT] pubsub 2.0 Oct 3, 2018
@codecov-io
Copy link

codecov-io commented Oct 4, 2018

Codecov Report

Merging #2532 into develop will decrease coverage by 0.04%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop    #2532      +/-   ##
===========================================
- Coverage    62.31%   62.26%   -0.05%     
===========================================
  Files          212      212              
  Lines        17099    17185      +86     
===========================================
+ Hits         10655    10701      +46     
- Misses        5547     5587      +40     
  Partials       897      897
Impacted Files Coverage Δ
libs/db/remotedb/grpcdb/server.go 0% <0%> (ø) ⬆️
consensus/reactor.go 66.89% <0%> (+0.23%) ⬆️
libs/autofile/autofile.go 78.94% <0%> (+2.63%) ⬆️
consensus/wal_generator.go 79.43% <0%> (+3.24%) ⬆️
privval/ipc_server.go 73.58% <0%> (+9.43%) ⬆️


Go channels are de-facto standard for carrying data between goroutines.

**Question: Does it worth switching to callback functions?**
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be: “Is it worth ...”


### Channels vs Callbacks

Yet another question is whenever we should use channels for message transmission or
Copy link
Contributor

Choose a reason for hiding this comment

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

... “question is whether we should” ... (?) or “in which cases we should...”

Copy link
Contributor

@liamsi liamsi left a comment

Choose a reason for hiding this comment

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

🥇 🚀 🍬

### Positive

- more idiomatic interface
- subscribers know what tags msg was published with
Copy link
Contributor

@liamsi liamsi Nov 1, 2018

Choose a reason for hiding this comment

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

nit: this could be made a whole sentence; e.g., something along the lines "adds the ability to subscribers to know with which associated tags a messages was published with"

@melekes melekes merged commit 80e4fe6 into develop Nov 2, 2018
@melekes melekes deleted the anton/pubsub-adr branch November 2, 2018 09:16
maxim-levy pushed a commit to maxim-levy/tendermint that referenced this pull request Nov 13, 2018
* 'master' of https://github.com/tendermint/tendermint: (330 commits)
  Release/v0.26.1 (tendermint#2803)
  fix amino overhead computation for Tx (tendermint#2792)
  p2p: re-check after sleeps (tendermint#2664)
  check the result of `ps.peer.Send` before calling `ps.setHasVote` (tendermint#2787)
  p2p: AddressBook requires addresses to have IDs; Do not close conn immediately after sending pex addrs in seed mode (tendermint#2797)
  test AutoFile#Size (happy path)
  [autofile/group] do not panic when checking size
  openFile creates a file if not exist => ErrNotExist is not possible
  use our logger in autofile/group
  Add tests for ValidateBasic methods (tendermint#2754)
  [docs] improve organization of ABCI docs & fix links (tendermint#2749)
  p2p: peer-id -> peer_id (tendermint#2771)
  mempool: print postCheck error (tendermint#2762)
  Fix crypto/merkle ProofOperators.Verify to check bounds on keypath pa… (tendermint#2756)
  Mempool WAL is still created by default in home directory, leads to permission errors (tendermint#2758)
  mempool: ErrPreCheck and more log info (tendermint#2724)
  Release/v0.26.0 (tendermint#2726)
  [ADR] [DRAFT] pubsub 2.0 (tendermint#2532)
  validate reactor messages (tendermint#2711)
  TMHASH is 32 bytes. Closes tendermint#1990 (tendermint#2732)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants