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.

Sign up for GitHub

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

Jump to bottom

indexer: allow to index an event at runtime #4466

Merged
merged 1 commit into from
Apr 22, 2020
Merged

indexer: allow to index an event at runtime #4466

merged 1 commit into from
Apr 22, 2020

Conversation

favadi
Copy link
Contributor

@favadi favadi commented Feb 24, 2020 •
edited
Loading

The PR added a new field index to event attribute, that will cause indexer service to index the event if set to true.

Fixes #3437.

  • Referenced an issue explaining the need for the change
  • Updated all relevant documentation in docs
  • Updated all code comments where relevant
  • Wrote tests
  • Updated CHANGELOG_PENDING.md

@favadi favadi marked this pull request as ready for review February 24, 2020 13:28
@erikgrinaker erikgrinaker self-assigned this Mar 2, 2020
erikgrinaker
erikgrinaker reviewed Mar 2, 2020
View reviewed changes
Copy link
Contributor

@erikgrinaker erikgrinaker left a comment

Choose a reason for hiding this comment

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

LGTM. Could you add a changelog entry please?

rpc/client/rpc_test.go Show resolved Hide resolved
@favadi favadi force-pushed the event-indexed-keys branch from a9f7349 to b4b121c Compare March 2, 2020 09:39
@favadi
Copy link
Contributor Author

favadi commented Mar 2, 2020

LGTM. Could you add a changelog entry please?

Changelog added.

erikgrinaker
erikgrinaker approved these changes Mar 2, 2020
View reviewed changes
Copy link
Contributor

@erikgrinaker erikgrinaker left a comment

Choose a reason for hiding this comment

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

Thank you! I'm a bit unsure whether we consider this a breaking change (nodes with and without this change will have differences in persisted data), so I'll leave it for the next minor release to be on the safe side.

@erikgrinaker erikgrinaker added the R:major PR contains breaking changes that have to wait till a major release is made to be merged label Mar 2, 2020
melekes
melekes suggested changes Mar 2, 2020
View reviewed changes
libs/kv/types.proto Outdated Show resolved Hide resolved
@favadi favadi force-pushed the event-indexed-keys branch from b4b121c to b9d6011 Compare March 8, 2020 09:37
@favadi favadi requested review from melekes and erikgrinaker March 8, 2020 09:39
@tessr tessr mentioned this pull request Mar 9, 2020
Merged
@melekes
Copy link
Contributor

melekes commented Mar 11, 2020 •
edited
Loading

EventAttribute name makes more sense to me than IndexPair. Pair was an abstract data type. That's why it was located in libs. Now we're adding implementation details to this type. That's why it makes sense to move it to abci/types.proto.

@favadi favadi force-pushed the event-indexed-keys branch from b9d6011 to e33eaa8 Compare March 15, 2020 10:52
@favadi
Copy link
Contributor Author

favadi commented Mar 15, 2020

@melekes I moved the IndexPair to abci/types.proto and renamed to EventAttribute. The kv.Pair is still required as it is (at least) being used in crypto/merkle.

@favadi favadi requested a review from melekes March 15, 2020 10:54
melekes
melekes approved these changes Mar 18, 2020
View reviewed changes
Copy link
Contributor

@melekes melekes left a comment

Choose a reason for hiding this comment

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

lgtm

libs/kv/types.proto Show resolved Hide resolved
@melekes melekes added the T:breaking Type: Breaking Change label Mar 18, 2020
alexanderbez
alexanderbez approved these changes Apr 3, 2020
View reviewed changes
Copy link
Contributor

@alexanderbez alexanderbez left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM (pending conflicts resolutions). I'd double-check that we're not missing documentation anywhere else. I do recall events/indexing docs to be spread amongst a few locations.

@favadi favadi force-pushed the event-indexed-keys branch from e33eaa8 to cab90c4 Compare April 3, 2020 16:44
@favadi
Copy link
Contributor Author

favadi commented Apr 3, 2020

PR rebased.

@tac0turtle
Copy link
Contributor

@melekes @erikgrinaker what do you guys think about renaming libs/kv to libs/event and bringing back the type to that directory? Otherwise we should remove kv.Pair as it is not used anywhere and the new type needs to implment the sorting interface as it is here: /~https://github.com/tendermint/tendermint/blob/master/libs/kv/kvpair.go

@melekes
Copy link
Contributor

melekes commented Apr 6, 2020

I like that EventAttribute is located in the same place where Event is.

@erikgrinaker
Copy link
Contributor

Now that 0.33.4 is out the door I think we can finally get this merged. Would you mind updating it with master @favadi?

@favadi
indexer: allow to index an event at runtime
a287611 
The PR added a new field `index` to event attribute, that will cause indexer service to index the event if set to true.
@favadi favadi force-pushed the event-indexed-keys branch from 0da832f to a287611 Compare April 22, 2020 09:19
@favadi
Copy link
Contributor Author

favadi commented Apr 22, 2020

@erikgrinaker I rebased the PR on master, tests are passing.

@erikgrinaker erikgrinaker merged commit 843d63f into tendermint:master Apr 22, 2020
@erikgrinaker
Copy link
Contributor

Fantastic, thank you!

@favadi favadi deleted the event-indexed-keys branch April 22, 2020 10:15
tac0turtle pushed a commit that referenced this pull request Apr 29, 2020
@favadi @tac0turtle
indexer: allow indexing an event at runtime (#4466)
e33476d 
The PR added a new field `index` to event attribute, that will cause indexer service to index the event if set to true.
@tac0turtle tac0turtle mentioned this pull request May 3, 2020
Closed
4 tasks
This was referenced May 24, 2020
Closed
Merged
Closed
mergify bot pushed a commit that referenced this pull request May 26, 2020
@tac0turtle
libs: remove kv (#4874)
89a3cb2 
## Description

In #4466 a new type was created (EventAttribute) that replaced KV.Pair. This made the type Pair deadcode, this pr removes that type.

I don't think a changelog entry is needed since it is not being used anymore. But will add a section to the upgrading.md to let users know that it was replaced

Closes: #XXX
@ebuchman ebuchman mentioned this pull request Jul 16, 2020
Closed
NexZhu pushed a commit to daotl/go-acei that referenced this pull request Oct 27, 2021
@tac0turtle
libs: remove kv (#4874)
e19e404 
## Description

In tendermint/tendermint#4466 a new type was created (EventAttribute) that replaced KV.Pair. This made the type Pair deadcode, this pr removes that type.

I don't think a changelog entry is needed since it is not being used anymore. But will add a section to the upgrading.md to let users know that it was replaced

Closes: #XXX
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tac0turtle tac0turtle tac0turtle left review comments

@alexanderbez alexanderbez alexanderbez approved these changes

@melekes melekes melekes approved these changes

@erikgrinaker erikgrinaker erikgrinaker approved these changes

@ebuchman ebuchman Awaiting requested review from ebuchman

@tessr tessr Awaiting requested review from tessr

Assignees

@erikgrinaker erikgrinaker

Labels
R:major PR contains breaking changes that have to wait till a major release is made to be merged T:breaking Type: Breaking Change
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Allow setting tags to index at runtime
5 participants
@favadi @melekes @tac0turtle @erikgrinaker @alexanderbez