kvindexer: Removing event function type as it is always the same #581

jmalicevic · 2023-03-24T14:58:24Z

Closes #571 .

Removes the event function type identifier form the event key. Earlier having this in the key made sense as it differentiated between BeginBlock and EndBlock events. With FinalizeBlock it will always be the same and has no point.

The only issue is that we need to support data indexed with old versions where the key has the function identifier key (see util.go:L123.

Note: I am piggybacking updates to the tests because there were duplicate UTs and UTs refering to match_events which do not exist in this version.

PR checklist

Tests written/updated
Backwards compatibility test
Run e2e upgrading test
Changelog entry added in .changelog (we use unclog to manage our changelog)
Updated relevant documentation (docs/ or spec/) and code comments

This reverts commit b647103.

…cometbft/cometbft into jasmina/571-kvindexer-remove-event-type

sergio-mena · 2023-04-04T18:36:56Z

state/indexer/block/kv/kv_test.go

-		"query return all events from a height - exact - no match.events": {
-			q:       query.MustCompile("block.height = 1"),
-			results: []int64{1},
-		},


I don't understand why these test cases are no longer needed

BEcause they are the exact duplicate of the existing tests - they were there because we used to test with and without the match-events keywords and then when I fixed the tests for not use the keyword, I did not delete the duplicate ones.

state/indexer/block/kv/util.go

sergio-mena · 2023-04-04T18:41:46Z

state/indexer/block/kv/util.go

+		if err != nil {                                       // If it cannot parse the event function type, it could be 1
+			remaining, err2 := orderedcode.Parse(string(key), &compositeKey, &eventValue, &height, &eventSeq)
+			if err2 != nil || len(remaining) != 0 { // We should not have anything else after the eventSeq
+				return 0, fmt.Errorf("failed to parse event key: %w", err)


Do we also want to report err2?

sergio-mena · 2023-04-04T18:41:56Z

state/indexer/block/kv/util.go

+		} else {
+			remaining, err2 := orderedcode.Parse(remaining2, &eventSeq) // If 2, , retrieve the eventSeq, otherwise ignore
+			if err2 != nil || len(remaining) != 0 {                     // We should not have anything else after the eventSeq
+				return 0, fmt.Errorf("failed to parse event key: %w", err)


Report err2?

sergio-mena · 2023-04-04T18:43:25Z

state/indexer/block/kv/util.go

+			}
+
+		} else {
+			remaining, err2 := orderedcode.Parse(remaining2, &eventSeq) // If 2, , retrieve the eventSeq, otherwise ignore


What does "If 2, ," mean?
Also, how do we ignore here? (I see we're returning the error if err2 is not nil)

If 2 - the case under number 2. above , that have both an event sequence and event type.
I will rephrase this, but essentially ignore means, that we do not have the event sequence, but rather just the event function type (case 3.), and we will ignore. Err2 should still be returned.

sergio-mena · 2023-04-04T18:47:51Z

state/txindex/kv/kv_test.go

 			q:             "account.number <= 2 AND account.owner CONTAINS '/Iv' AND account.owner CONTAINS 'an' AND tx.height = 1",
 			resultsLength: 1,
 		},
-		" Match range with match events contains": {


Are we testing all "if" branches in the changes to file state/indexer/block/kv/util.go above?

I could not think of how to test the cases with UTs because those if branches are handling events indexed using old versions of the indexer. Unless we create different indexing options in our UTs. We could provide a custom indexing function to generate the event key differently and then test. That is basically what i did manually. And I did this with the backwards compatibility tests when running on top of data indexed with old versions.

Co-authored-by: Sergio Mena <sergio@informal.systems>

state/indexer/block/kv/util.go

sergio-mena · 2023-04-11T17:10:01Z

state/indexer/block/kv/util.go

+			remaining, err2 := orderedcode.Parse(remaining2, &eventSeq) // If the event follows the scenario in 2.,
+			// retrieve the eventSeq, otherwise, there should be no remainder
+			// and no error and this parsing has no effect.
+			if err2 != nil || len(remaining) != 0 { // We should not have anything else after the eventSeq if in 2.
+				return 0, fmt.Errorf("failed to parse event key: %w", err2)
+			}


Suggested change

remaining, err2 := orderedcode.Parse(remaining2, &eventSeq) // If the event follows the scenario in 2.,

// retrieve the eventSeq, otherwise, there should be no remainder

// and no error and this parsing has no effect.

if err2 != nil || len(remaining) != 0 { // We should not have anything else after the eventSeq if in 2.

return 0, fmt.Errorf("failed to parse event key: %w", err2)

}

if len(remaining2) != 0 { // Are we in case 2 or 3

remaining, err2 := orderedcode.Parse(remaining2, &eventSeq) // the event follows the scenario in 2.,

// retrieve the eventSeq

// there should be no error

if err2 != nil { // We should not have anything else after the eventSeq if in 2.

return 0, fmt.Errorf("failed to parse event key: %w", err2)

}

}

I'm suggesting this, because I don't clearly see the path taken by scenario 3 in the present code.
I also see two main advantages with my suggestion:

Easier to distinguish paths followed by scenarios 2 and 3

You don't call Parse unless you're sure you need to call it (cleaner)

I am fine with the chaneg but we still need to check whether len(remaining) != 0 because that is invalid. I will push a change including both. Let me know if you agree.

thanethomson · 2023-04-20T10:07:51Z

.changelog/unreleased/improvements/581-remove-event-function-type-from-indexer.md

+- `[kvindex]` Remove event function type from event key in kvindexer. The function type is not always `FinalizeBlock`.
+  ([\#581](https://github.com/cometbft/cometbft/pull/581))


What's the impact here on the user? If there's no direct impact on the user, it's not clear to me that we'd need a changelog entry. If the overall goal is to reduce storage consumption, then I'd recommend rewording to something that explicitly highlights that. Users may not understand what this changelog entry means for them otherwise.

Also, if it's being backported to v0.38, then the PR to main shouldn't have a changelog entry - only the backport PR should have a changelog entry (from a user's perspective, they'd be receiving this change in the v0.38 release series, so there'd be no need to also be notified about it in the v0.39 series).

Makes sense. I will remove the changelog entry here. As for the impact on the user, there is no direct impact as far as I can see. We have info on this in the upgrading docs for 0.38 because the format of data on disk changes. However, people who have forks that play with the indexer, they might be impacted.

state/indexer/block/kv/util.go

sergio-mena

Thanks for the refactoring of the intricate code!

jmalicevic · 2023-04-27T22:28:07Z

Thank you guys for improving it! @lasarojc , let me know if you are happy with the changes and I can merge this :)

@lasarojc

* Applied @serigo-menas PR comments * Removed changelog entry * Refactored code according to suggestion from @lasarojc --------- Co-authored-by: Sergio Mena <sergio@informal.systems> Co-authored-by: Lásaro Camargos <lasaro@informal.systems> (cherry picked from commit e9e9cf3)

@lasarojc

…kport #581) (#774) * kvindexer: Removing event function type as it is always the same (#581) * Applied @serigo-menas PR comments * Removed changelog entry * Refactored code according to suggestion from @lasarojc --------- Co-authored-by: Sergio Mena <sergio@informal.systems> Co-authored-by: Lásaro Camargos <lasaro@informal.systems> (cherry picked from commit e9e9cf3) * Added changelog * Update .changelog/unreleased/breaking-changes/774-state-indexerevent-remove-function-type.md Co-authored-by: Thane Thomson <connect@thanethomson.com> --------- Co-authored-by: Jasmina Malicevic <jasmina.dustinac@gmail.com> Co-authored-by: Thane Thomson <connect@thanethomson.com>

@lasarojc

* Applied @serigo-menas PR comments * Removed changelog entry * Refactored code according to suggestion from @lasarojc --------- Co-authored-by: Sergio Mena <sergio@informal.systems> Co-authored-by: Lásaro Camargos <lasaro@informal.systems>

jmalicevic added 2 commits March 24, 2023 15:54

Initial working code

3813fc5

Removed backwards compatibility check

06edbee

jmalicevic added indexer storage labels Mar 24, 2023

jmalicevic added this to the 2023-Q1 milestone Mar 24, 2023

jmalicevic self-assigned this Mar 24, 2023

jmalicevic linked an issue Mar 24, 2023 that may be closed by this pull request

kvindexer: Remove event type from block event #571

Closed

2 tasks

jmalicevic added 6 commits March 27, 2023 11:03

refactored code

0bfa193

Code refactoring

b647103

Revert "Code refactoring"

e0e96f0

This reverts commit b647103.

Fixed typo

bbb9450

Added changelog

77454bf

Merge branch 'main' into jasmina/571-kvindexer-remove-event-type

53f3d1a

jmalicevic marked this pull request as ready for review March 27, 2023 11:19

jmalicevic requested a review from a team as a code owner March 27, 2023 11:19

Merge branch 'main' into jasmina/571-kvindexer-remove-event-type

42ea15f

jmalicevic marked this pull request as draft March 29, 2023 13:19

jmalicevic added 3 commits April 3, 2023 21:53

Merge branch 'main' into jasmina/571-kvindexer-remove-event-type

b2d3919

Refactored code and tests

9843fe5

Merge branch 'jasmina/571-kvindexer-remove-event-type' of github.com:…

95be946

…cometbft/cometbft into jasmina/571-kvindexer-remove-event-type

jmalicevic added the backport-to-v0.38.x Tell Mergify to backport the PR to v0.38.x label Apr 4, 2023

jmalicevic marked this pull request as ready for review April 4, 2023 08:20

Fixed typo

3102157

sergio-mena reviewed Apr 4, 2023

View reviewed changes

jmalicevic and others added 3 commits April 6, 2023 11:00

Apply suggestions from code review

a8282e9

Co-authored-by: Sergio Mena <sergio@informal.systems>

Fixed linter

4dd90a4

Merge branch 'main' into jasmina/571-kvindexer-remove-event-type

a37c695

thanethomson reviewed Apr 7, 2023

View reviewed changes

state/indexer/block/kv/util.go Show resolved Hide resolved

jmalicevic added 2 commits April 11, 2023 11:58

Merge branch 'main' into jasmina/571-kvindexer-remove-event-type

9052985

Merge branch 'main' into jasmina/571-kvindexer-remove-event-type

dbc7020

sergio-mena reviewed Apr 11, 2023

View reviewed changes

state/indexer/block/kv/util.go Show resolved Hide resolved

sergio-mena reviewed Apr 11, 2023

View reviewed changes

state/indexer/block/kv/util.go Show resolved Hide resolved

sergio-mena reviewed Apr 11, 2023

View reviewed changes

thanethomson modified the milestones: 2023-Q1, 2023-Q2 Apr 13, 2023

jmalicevic added 3 commits April 17, 2023 16:26

Merge branch 'main' into jasmina/571-kvindexer-remove-event-type

6a818f6

Applied @serigo-menas PR comments

3803f2b

Merge branch 'main' into jasmina/571-kvindexer-remove-event-type

ddf8322

thanethomson reviewed Apr 20, 2023

View reviewed changes

Removed changelog entry

af0578e

lasarojc reviewed Apr 24, 2023

View reviewed changes

state/indexer/block/kv/util.go Show resolved Hide resolved

lasarojc reviewed Apr 24, 2023

View reviewed changes

state/indexer/block/kv/util.go Outdated Show resolved Hide resolved

Refactored code according to suggestion from @lasarojc

f2d6d0e

sergio-mena approved these changes Apr 27, 2023

View reviewed changes

Merge branch 'main' into jasmina/571-kvindexer-remove-event-type

9dc90fc

lasarojc approved these changes Apr 28, 2023

View reviewed changes

jmalicevic merged commit e9e9cf3 into main Apr 28, 2023

jmalicevic deleted the jasmina/571-kvindexer-remove-event-type branch April 28, 2023 19:47

mergify bot mentioned this pull request Apr 28, 2023

kvindexer: Removing event function type as it is always the same (backport #581) #774

Merged

faddat mentioned this pull request Oct 15, 2024

chore: use latest cometbft-db in v0.38.x #4296

Closed

3 tasks

		- `[kvindex]` Remove event function type from event key in kvindexer. The function type is not always `FinalizeBlock`.
		([\#581](https://github.com/cometbft/cometbft/pull/581))

kvindexer: Removing event function type as it is always the same #581

kvindexer: Removing event function type as it is always the same #581

Uh oh!

Conversation

jmalicevic commented Mar 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

sergio-mena left a comment

Choose a reason for hiding this comment

Uh oh!

jmalicevic commented Apr 27, 2023

Uh oh!

Uh oh!

jmalicevic commented Mar 24, 2023 •

edited

Loading