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

Add IndexerDb.Close() #801

Merged
merged 7 commits into from
Dec 8, 2021
Merged

Add IndexerDb.Close() #801

merged 7 commits into from
Dec 8, 2021

Conversation

tolikzinovyev
Copy link
Contributor

Summary

When running test with the --test-pg flag, we open many database connections without closing them. This leads to the database refusing new connections.

This PR adds IndexerDb.Close() function that closes all active connections to the database.

@codecov-commenter
Copy link

codecov-commenter commented Nov 30, 2021

Codecov Report

Merging #801 (3103b54) into develop (f372206) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #801      +/-   ##
===========================================
+ Coverage    58.83%   58.84%   +0.01%     
===========================================
  Files           29       29              
  Lines         4059     4060       +1     
===========================================
+ Hits          2388     2389       +1     
  Misses        1374     1374              
  Partials       297      297              
Impacted Files Coverage Δ
idb/idb.go 51.21% <ø> (ø)
idb/postgres/postgres.go 47.68% <100.00%> (+0.03%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f372206...3103b54. Read the comment docs.

@winder winder self-requested a review December 2, 2021 20:52
Copy link
Contributor

@winder winder left a comment

Choose a reason for hiding this comment

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

Looks good, just a couple small things to address.

Comment on lines +106 to +110
imp := importer.NewImporter(db)
handler := func(ctx context.Context, block *rpcs.EncodedBlockCert) error {
return handleBlock(block, &imp)
}
bot.SetBlockHandler(handler)
Copy link
Contributor

@winder winder Dec 7, 2021

Choose a reason for hiding this comment

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

nit: I slightly prefer the interface object over the closure here (up to you though).

AddBlockHandler -> SetBlockHandler is a nice simplification

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why? Interface objects are harder to construct at the user side.

Copy link
Contributor

Choose a reason for hiding this comment

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

The terseness mainly, you could create/set it in one line if you wanted. Here you must create the importer object separately.

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'm not sure about terseness. With a closure, looking at the function signature, you immediately see what the closure is. Plus, an interface is a set of functions. That's redundant.

fetcher/fetcher.go Outdated Show resolved Hide resolved
fetcher/fetcher.go Outdated Show resolved Hide resolved
Comment on lines +223 to +227
return fmt.Errorf("Run() err: %w", err)
case err := <-ch1:
cancelFunc()
<-ch0
return fmt.Errorf("Run() err: %w", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Disambiguate the errors, more descriptive channel names would also be helpful.

Suggested change
return fmt.Errorf("Run() err: %w", err)
case err := <-ch1:
cancelFunc()
<-ch0
return fmt.Errorf("Run() err: %w", err)
return fmt.Errorf("Run() processQueue err: %w", err)
case err := <-ch1:
cancelFunc()
<-ch0
return fmt.Errorf("Run() mainLoop err: %w", err)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the err objects will tell where they come from

Copy link
Contributor

Choose a reason for hiding this comment

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

the err objects will should tell where they come from

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But they do. You can check mainLoop() and processQueue().

Copy link
Contributor

@winder winder left a comment

Choose a reason for hiding this comment

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

Looks good

@tolikzinovyev tolikzinovyev merged commit 7655717 into develop Dec 8, 2021
@tolikzinovyev tolikzinovyev deleted the tolik/close-db branch December 8, 2021 20:32
@tolikzinovyev
Copy link
Contributor Author

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants