Skip to content
This repository was archived by the owner on Mar 14, 2025. It is now read-only.

CCIP-1306 Using proper query for fetching single CommitReport from the LP#291

Merged
mateusz-sekara merged 5 commits intoccip-developfrom
commit-reports/proper-fetch
Nov 17, 2023
Merged

CCIP-1306 Using proper query for fetching single CommitReport from the LP#291
mateusz-sekara merged 5 commits intoccip-developfrom
commit-reports/proper-fetch

Conversation

@mateusz-sekara
Copy link
Contributor

@mateusz-sekara mateusz-sekara commented Nov 16, 2023

Motivation

The current approach to fetching CommitReport based on seqNr is suboptimal. We can have query with better filters that pick exactly one report that we need instead of fetching all Commit Reports greater or equal passed sequence number. Under higher load it becomes a bottleneck

Solution

Use a dedicated query LogsDataWordBetween that applies lower and upper bound filters in the SQL query

@mateusz-sekara mateusz-sekara requested a review from a team as a code owner November 16, 2023 09:55
@mateusz-sekara mateusz-sekara changed the title Using proper query for fetching single CommitReport from the LP CCIP-1306 Using proper query for fetching single CommitReport from the LP Nov 16, 2023
@mateusz-sekara mateusz-sekara force-pushed the commit-reports/proper-fetch branch from f07a917 to dbec116 Compare November 16, 2023 10:26
Copy link
Contributor

@dimkouv dimkouv left a comment

Choose a reason for hiding this comment

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

love this!
added some nits

func (c *CommitStoreV1_0_0) GetAcceptedCommitReportsGteSeqNum(ctx context.Context, seqNum uint64, confs int) ([]Event[CommitStoreReport], error) {
logs, err := c.lp.LogsDataWordGreaterThan(
func (c *CommitStoreV1_0_0) GetCommitReportMatchingSeqNum(ctx context.Context, seqNum uint64, confs int) ([]Event[CommitStoreReport], error) {
logs, err := c.lp.LogsDataWordBetween(
Copy link
Collaborator

Choose a reason for hiding this comment

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

we should be erroring if len(logs) > 1 to comply with the interface comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added defensive check + log without returning an error. If we get more than one it's probably a scenario which will be hard to recover from, I assume it might be some glitch/error in LP - for instance, it persisted the same Log twice or sth. Let's be defensive here, but also not break processing. Therefore I added log and returned always at most 1 element slice

}
}

func Benchmark_LogsDataWordBetween(b *testing.B) {
Copy link
Contributor Author

@mateusz-sekara mateusz-sekara Nov 17, 2023

Choose a reason for hiding this comment

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

@dimkouv No big difference ;) 1-2 ms on avg, let's wait with adding limit1 until we have some DSL to do that

Using LIMIT 1

Benchmark_LogsDataWordBetween
Benchmark_LogsDataWordBetween-12    	      42	  26292686 ns/op
Benchmark_LogsDataWordBetween-12    	      40	  26545705 ns/op
Benchmark_LogsDataWordBetween-12    	      42	  25068981 ns/op
Benchmark_LogsDataWordBetween-12    	      42	  24866132 ns/op
Benchmark_LogsDataWordBetween-12    	      43	  25001792 ns/op
Benchmark_LogsDataWordBetween-12    	      43	  25912275 ns/op
Benchmark_LogsDataWordBetween-12    	      43	  24767023 ns/op
Benchmark_LogsDataWordBetween-12    	      42	  26012244 ns/op
Benchmark_LogsDataWordBetween-12    	      48	  25809934 ns/op
Benchmark_LogsDataWordBetween-12    	      42	  24808771 ns/op
PASS

Without LIMIT

Benchmark_LogsDataWordBetween
Benchmark_LogsDataWordBetween-12    	      42	  26762404 ns/op
Benchmark_LogsDataWordBetween-12    	      43	  27254803 ns/op
Benchmark_LogsDataWordBetween-12    	      39	  28243682 ns/op
Benchmark_LogsDataWordBetween-12    	      43	  27615336 ns/op
Benchmark_LogsDataWordBetween-12    	      39	  27010718 ns/op
Benchmark_LogsDataWordBetween-12    	      43	  26643946 ns/op
Benchmark_LogsDataWordBetween-12    	      44	  26516652 ns/op
Benchmark_LogsDataWordBetween-12    	      39	  34758611 ns/op
Benchmark_LogsDataWordBetween-12    	      40	  25562183 ns/op
Benchmark_LogsDataWordBetween-12    	      42	  26939871 ns/op
PASS

@mateusz-sekara mateusz-sekara merged commit 84a60b8 into ccip-develop Nov 17, 2023
@mateusz-sekara mateusz-sekara deleted the commit-reports/proper-fetch branch November 17, 2023 12:29
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants