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

Preparations for removing BaseDendrite #3016

Merged
merged 15 commits into from
Mar 17, 2023
Merged

Conversation

S7evinK
Copy link
Contributor

@S7evinK S7evinK commented Mar 16, 2023

Preparations to actually remove/replace BaseDendrite.
Quite a few changes:

  • SyncAPI accepts an fulltext.Indexer interface (fulltext is removed from BaseDendrite)
  • Caches are removed from BaseDendrite
  • Introduces a Router struct (likely to change)
  • Introduces a sqlutil.ConnectionManager, which should remove base.DatabaseConnection later on
  • probably more

@S7evinK S7evinK added the T-Task Refactoring, removal, replacement, enabling or disabling functionality, other engineering tasks. label Mar 16, 2023
@S7evinK S7evinK changed the title Preparations for removing `BaseDendrite´ Preparations for removing BaseDendrite Mar 16, 2023
@codecov
Copy link

codecov bot commented Mar 16, 2023

Codecov Report

Patch coverage: 86.47% and project coverage change: +0.10 🎉

Comparison is base (d88f71a) 63.04% compared to head (96590e4) 63.15%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3016      +/-   ##
==========================================
+ Coverage   63.04%   63.15%   +0.10%     
==========================================
  Files         496      497       +1     
  Lines       53837    53843       +6     
==========================================
+ Hits        33944    34003      +59     
+ Misses      16173    16115      -58     
- Partials     3720     3725       +5     
Flag Coverage Δ
unittests 43.01% <86.47%> (+0.06%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
cmd/dendrite/main.go 84.84% <ø> (+5.11%) ⬆️
internal/caching/impl_ristretto.go 90.78% <ø> (ø)
syncapi/consumers/clientapi.go 33.33% <ø> (ø)
syncapi/consumers/roomserver.go 59.69% <ø> (ø)
syncapi/routing/routing.go 47.96% <ø> (ø)
syncapi/routing/search.go 0.00% <0.00%> (ø)
setup/mscs/msc2836/storage.go 45.48% <42.85%> (ø)
syncapi/storage/sqlite3/syncserver.go 50.86% <50.00%> (+0.42%) ⬆️
syncapi/syncapi.go 59.09% <53.84%> (-3.88%) ⬇️
federationapi/storage/postgres/storage.go 47.05% <66.66%> (ø)
... and 32 more

... and 46 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Member

@kegsay kegsay left a comment

Choose a reason for hiding this comment

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

Great improvement so far! Let's land it.

// Create required internal APIs
rsAPI := roomserver.NewInternalAPI(base)
rsAPI := roomserver.NewInternalAPI(base, caches)
Copy link
Member

Choose a reason for hiding this comment

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

I assume the idea is unpick base from this entirely, and adding caches is just a step towards that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, that is the idea. Gradually remove the need for base.

httpRouter.PathPrefix(httputil.PublicClientPathPrefix).Handler(base.PublicClientAPIMux)
httpRouter.PathPrefix(httputil.PublicMediaPathPrefix).Handler(base.PublicMediaAPIMux)
httpRouter.PathPrefix(httputil.PublicClientPathPrefix).Handler(base.Routers.Client)
httpRouter.PathPrefix(httputil.PublicMediaPathPrefix).Handler(base.Routers.Media)
Copy link
Member

Choose a reason for hiding this comment

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

Amazing names, much much better now we have no private APIs 😍

go func() {
<-ctx.Done()
_ = fts.Close()
}()
Copy link
Member

Choose a reason for hiding this comment

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

Needs docs.

"net/url"

"github.com/gorilla/mux"
Copy link
Member

Choose a reason for hiding this comment

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

As a side note, do we have an issue to replace mux? We need to as it has been archived and isn't maintained anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now we have #3017

)

type ConnectionManager interface {
Connection(dbProperties *config.DatabaseOptions) (*sql.DB, Writer, error)
Copy link
Member

Choose a reason for hiding this comment

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

Not sure I see the value in interfacing this? Any concrete impl would need to return a DB which is pretty difficult to mock out!

@S7evinK S7evinK marked this pull request as ready for review March 17, 2023 10:47
@S7evinK S7evinK requested a review from a team as a code owner March 17, 2023 10:47
@S7evinK S7evinK enabled auto-merge (squash) March 17, 2023 10:53
@S7evinK S7evinK merged commit 5579121 into main Mar 17, 2023
@S7evinK S7evinK deleted the s7evink/shrinkbasedendrite branch March 17, 2023 11:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-Task Refactoring, removal, replacement, enabling or disabling functionality, other engineering tasks.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dendrite returns improper error codes for unknown endpoints
2 participants