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-038 Implementation #10096

Closed
4 of 11 tasks
i-norden opened this issue Sep 7, 2021 · 13 comments
Closed
4 of 11 tasks

ADR-038 Implementation #10096

i-norden opened this issue Sep 7, 2021 · 13 comments

Comments

@i-norden
Copy link
Contributor

i-norden commented Sep 7, 2021

This is a parent issue to track progress on the ADR-038 state listening epic

Original proposals/issues:

Related issues:

Related discussions:

For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@robert-zaremba
Copy link
Collaborator

For the part 3, instead of building gRPC service to stream file changes and reinventing a wheel let's use a proper queue service in the first place like Kafka or RabbitMQ. These are market solutions, widely used, with many integrations.

@i-norden
Copy link
Contributor Author

i-norden commented Sep 24, 2021

It's a bit frustrating to have this design flip-flop occur after the file streaming service implementation has been open and undergoing review since February. If this had been brought up during the design phase we could have avoided this unnecessary work (it also voids my work done on the 3rd part of the ADR here).

This was discussed in depth before the ADR was finalized and merged- the file writing service is included as part of the ADR. A lot of the discussion in regards to the "base" implementation can be here and I think Bez's comment here is especially pertinent.

That said, I recognize the appeal and am open to making this change! Of the two listed, I am partial to the push-based approach of RabbitMQ.

@i-norden
Copy link
Contributor Author

i-norden commented Sep 24, 2021

From today's call:

  1. Still merge feat: ADR-038 Part 2: StreamingService interface, file writing implementation, and configuration #8664
  2. Switch to plugin architecture
  3. Don't reopen feat: ADR-038 Part 3: State change files auxiliary gRPC server #9647 (or do we still want to introduce this PR and get a PoC writing to Postgres using it?)
  4. Introduce a Kafka or RabitMQ streaming service (consensus seems to be to use Kafka- using a sink connector or a plugin to enable a push consumer model)

@egaxhaj
Copy link
Contributor

egaxhaj commented Oct 5, 2021

Hi, @i-norden. I’m new to the Provenance team and have experience with Kafka. I want to help out on this issue. You can DM me on the Cosmos Developers Discord server as Ergels Gaxhaj | Provenance or we can have a discussion here.

@IbrarMakaveli
Copy link

Hey,

I'm a Data Engineer at Osmosis, I had a meeting today on this module that we are interested for the Streaming part, and indeed I am also in favor of migrating to Kafka for several reasons:

  • Kafka is resilient, fault tolerant, scalable and one of the leaders in the streaming market (core product is free on Apache licence)
  • It avoids you to maintain the code of the Producer & Consumer functions (which is very complicated).
  • Kafka has a large number of connectors, which makes integration very easy: Producer can be written in Go and Consumer write in Python, Java or whatever

@clevinson clevinson added the Epic label Oct 15, 2021
@clevinson clevinson added this to the v0.45 milestone Oct 15, 2021
@clevinson
Copy link
Contributor

@i-norden on today's SDK call we discussed this epic landing in v0.45 (which we're hoping to be feature complete in 2-4 weeks).

Does that timeline sound reasonable to you? I don't think we're wanting to push the release further than necessary, but there was a lot of desire to get this work merged in asap, so we wanted to see if this work can potentially get wrapped up in time for v0.45 under the current timeline.

@i-norden
Copy link
Contributor Author

@clevinson I think that is a reasonable timeline, the plugin architecture PR is ready to open once the current pending PR gets merged. So the plugin arch with a file streaming service plugin will definitely be ready. @egaxhaj-figure has taken ownership of the Kafka plugin work- does this timeline seem reasonable to you?

@egaxhaj
Copy link
Contributor

egaxhaj commented Oct 18, 2021

@clevinson, @i-norden Yes, 2-4 weeks is a reasonable timeline to get the Kafka plugin in there.

@robert-zaremba
Copy link
Collaborator

robert-zaremba commented Oct 19, 2021

Thanks @egaxhaj-figure . This is really needed.
Let's focus on merging part-2. It LGTM. Need @alexanderbez to release the block or add more comments. Once this is done, I suggest:

  • review and merge plugin interfaces --let's firstly do small PR and quick review rather then going into a big one. @i-norden are you still handling it? I can't see that PR. @egaxhaj-figure - would you collaborate on that?
  • review and merge Kafka based on the work above.
  • Part-3 still make sense feat: ADR-038 Part 3: State change files auxiliary gRPC server #9647 -- I've reopened it, but it's not in the priority now.

@robert-zaremba
Copy link
Collaborator

@IbrarMakaveli -- this was exactly the motivation for moving into the plugin architecture: to reuse existing queuing solutions.

File based approach is still an interesting thing and will be good to merge that functionality.

@i-norden
Copy link
Contributor Author

i-norden commented Oct 20, 2021

  • review and merge plugin interfaces --let's firstly do small PR and quick review rather then going into a big one. @i-norden are you still handling it? I can't see that PR. @egaxhaj-figure - would you collaborate on that?

Hey, yeah I'll handle the plugin arch PR. The PR isn't open against the master branch yet, will open it once this 2nd PR is merged (i-norden#1).

Thanks!

@robert-zaremba
Copy link
Collaborator

Discussed with @i-norden . Let's:

  • Do an ADR-38 PR update first (the architecture doc) to provide details about plugin architecture
  • Live review it next week,
  • The do the implementation PR

@clevinson clevinson modified the milestone: v0.45 Oct 29, 2021
mergify bot pushed a commit that referenced this issue Nov 27, 2021
For #10096 

This PR introduces the updates to the ADR-038 spec for the transition to plugin-based streaming services. These updates reflect the implementation approach taken in i-norden#1 (will be rebased, retargeted, and reopened). 

### Author Checklist

*All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.*

I have...

- [x] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [x] added `!` to the type prefix if API or client breaking change
- [x] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting))
- [x] provided a link to the relevant issue or specification
- [x] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules)
- [x] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing)
- [x] added a changelog entry to `CHANGELOG.md`
- [x] included comments for [documenting Go code](https://blog.golang.org/godoc)
- [x] updated the relevant documentation or specification
- [ ] reviewed "Files changed" and left comments if necessary
- [ ] confirmed all CI checks have passed

### Reviewers Checklist

*All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.*

I have...

- [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] confirmed `!` in the type prefix if API or client breaking change
- [ ] confirmed all author checklist items have been addressed 
- [ ] reviewed state machine logic
- [ ] reviewed API design and naming
- [ ] reviewed documentation is accurate
- [ ] reviewed tests and test coverage
- [ ] manually tested (if applicable)
blewater pushed a commit to e-money/cosmos-sdk that referenced this issue Dec 8, 2021
For cosmos#10096 

This PR introduces the updates to the ADR-038 spec for the transition to plugin-based streaming services. These updates reflect the implementation approach taken in i-norden#1 (will be rebased, retargeted, and reopened). 

### Author Checklist

*All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.*

I have...

- [x] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [x] added `!` to the type prefix if API or client breaking change
- [x] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting))
- [x] provided a link to the relevant issue or specification
- [x] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules)
- [x] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing)
- [x] added a changelog entry to `CHANGELOG.md`
- [x] included comments for [documenting Go code](https://blog.golang.org/godoc)
- [x] updated the relevant documentation or specification
- [ ] reviewed "Files changed" and left comments if necessary
- [ ] confirmed all CI checks have passed

### Reviewers Checklist

*All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.*

I have...

- [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] confirmed `!` in the type prefix if API or client breaking change
- [ ] confirmed all author checklist items have been addressed 
- [ ] reviewed state machine logic
- [ ] reviewed API design and naming
- [ ] reviewed documentation is accurate
- [ ] reviewed tests and test coverage
- [ ] manually tested (if applicable)
@tac0turtle tac0turtle removed the Epic label May 9, 2022
@tac0turtle tac0turtle removed this from the v0.46 milestone May 12, 2022
@tac0turtle
Copy link
Member

closing as adr38 in the current form is delivered. Work on a minor change is ongoing

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

No branches or pull requests

7 participants