Skip to content

Conversation

@roji
Copy link
Member

@roji roji commented Aug 5, 2022

  • Allow BuildMultiHost with one host and Build with multple.
  • Add AddNpgsqlMultiHostDatasource to Npgsql.DependencyInjection.

Closes #4578

@roji roji requested a review from NinoFloris August 5, 2022 08:45
@roji roji requested a review from vonzshik as a code owner August 5, 2022 08:45
@roji roji force-pushed the MultiHostDataSourceFixes branch 3 times, most recently from dd4493a to 213f741 Compare October 6, 2022 18:00
@roji
Copy link
Member Author

roji commented Oct 6, 2022

@NinoFloris @vonzshik the test issues are fixed, this is ready for reviewing.

FYI I tried to look at the naming discussion agin, around NpgsqlMultiHostDatasource.For. I tried to rename to e.g. WithTargetSessionAttributes(), but that means the typical API invocation would look like this:

var x = dataSource.WithTargetSessionAttributes(TargetSessionAttributes.Primary);

... which seems really needlessly repetitive and long (the argument is very likely to be an enum literal, which makes things very explicit). So for now I still think that although For is very vague, Intellisense and XML docs should be sufficient to make people understand what it's for...

Copy link
Contributor

@vonzshik vonzshik left a comment

Choose a reason for hiding this comment

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

Seems OK, though there seems to be an issue with one of the tests.

@roji roji force-pushed the MultiHostDataSourceFixes branch from 213f741 to b6a4462 Compare October 20, 2022 11:12
@roji roji enabled auto-merge (squash) October 20, 2022 11:13
@roji roji force-pushed the MultiHostDataSourceFixes branch from b6a4462 to 5d56c72 Compare October 31, 2022 07:25
* Allow BuildMultiHost with one host and Build with multple.
* Add AddNpgsqlMultiHostDatasource to Npgsql.DependencyInjection.
* All tests which use pg_terminate_backend are now marked as
  NonParallelizable and reset the cluster state cache, to prevent
  interference with multihost tests.

Closes npgsql#4578
@roji roji force-pushed the MultiHostDataSourceFixes branch from 5d56c72 to c291e48 Compare November 1, 2022 09:42
@roji roji merged commit 4482112 into npgsql:main Nov 1, 2022
@roji roji deleted the MultiHostDataSourceFixes branch November 1, 2022 09:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Reexamine the data source API for multiple hosts

2 participants