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.

Sign up for GitHub

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

Jump to bottom

Background primary determiner #121 #171

Merged
merged 18 commits into from
Oct 16, 2022
Merged

Background primary determiner #121 #171

merged 18 commits into from
Oct 16, 2022

Conversation

Evreke
Copy link
Collaborator

@Evreke Evreke commented Sep 7, 2022 •
edited
Loading

This feature makes the HighAvailabilityPgConnectionImpl responsible for monitoring and dynamically changing reference to the primary postgres node in the cluster.

@Evreke
Background primary determiner #121
0809313 
feat: reference change for connection to primary node by background task
fix: cluster container names must be unique, so they are generated using the uuid
Signed-off-by: Antipin Alexey <evreke@gmail.com>
@Evreke Evreke requested a review from mfvanek September 7, 2022 07:17
@Evreke Evreke changed the title feature/background-primary-determiner Background primary determiner #121 Sep 7, 2022
@codecov
Copy link

codecov bot commented Sep 7, 2022 •
edited
Loading

Codecov Report

Base: 100.00% // Head: 100.00% // No change to project coverage 👍

Coverage data is based on head (55d1223) compared to base (a508bc3).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files 
@@             Coverage Diff             @@
##              master      #171   +/-   ##
===========================================
  Coverage     100.00%   100.00%           
- Complexity       720       727    +7     
===========================================
  Files            110       110           
  Lines           1520      1542   +22     
  Branches         119       119           
===========================================
+ Hits            1520      1542   +22
Impacted Files Coverage Δ
...g/connection/HighAvailabilityPgConnectionImpl.java 100.00% <100.00%> (ø)

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 at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

mfvanek
mfvanek requested changes Sep 7, 2022
View reviewed changes
src/main/java/io/github/mfvanek/pg/connection/HighAvailabilityPgConnectionImpl.java Show resolved Hide resolved
src/main/java/io/github/mfvanek/pg/connection/HighAvailabilityPgConnectionImpl.java Outdated Show resolved Hide resolved
src/main/java/io/github/mfvanek/pg/connection/HighAvailabilityPgConnectionImpl.java Outdated Show resolved Hide resolved
src/main/java/io/github/mfvanek/pg/connection/HighAvailabilityPgConnectionImpl.java Outdated Show resolved Hide resolved
src/main/java/io/github/mfvanek/pg/connection/HighAvailabilityPgConnectionImpl.java Outdated Show resolved Hide resolved
src/main/java/io/github/mfvanek/pg/connection/HighAvailabilityPgConnectionImpl.java Outdated Show resolved Hide resolved
src/main/java/io/github/mfvanek/pg/connection/HighAvailabilityPgConnectionImpl.java Show resolved Hide resolved
src/test/java/io/github/mfvanek/pg/support/PostgresSqlClusterWrapper.java Outdated Show resolved Hide resolved
src/test/java/io/github/mfvanek/pg/connection/HighAvailabilityPgConnectionClusterTest.java Outdated Show resolved Hide resolved
src/test/java/io/github/mfvanek/pg/connection/HighAvailabilityPgConnectionClusterTest.java Outdated Show resolved Hide resolved
mfvanek and others added 9 commits September 30, 2022 11:10
@mfvanek @Evreke
Feature/generate migration to remove serial types (#172)
fd6d84c 
* DbMigrationGenerator was made generic

* Added ColumnWithSerialTypeMigrationGenerator

* Added tests

* Updated deps

* Fixed code smells

Co-authored-by: Ivan Vakhrushev <i.vakhrushev@banki.ru>
@mfvanek @Evreke
Signing via GPG 2+ (#173)
5063601 
Co-authored-by: Ivan Vakhrushev <mfvanek@gmail.com>
@mfvanek @Evreke
Making abstract classes package private (#174)
f35e23b 
* Making abstract classes package private

* Fixed code smell

Co-authored-by: Ivan Vakhrushev <i.vakhrushev@banki.ru>
@mfvanek @Evreke
Release v0.6.2 (#175)
2f159ba 
Co-authored-by: Ivan Vakhrushev <i.vakhrushev@banki.ru>
@mfvanek @Evreke
Prepare for the next development iteration (#176)
7ebd87e 
Co-authored-by: Ivan Vakhrushev <i.vakhrushev@banki.ru>
@mfvanek @Evreke
Wrong detection of serial types on columns with multiple constraints (#…
1c895c9 
…179)

* Updated sql query

* Added test to cover the issue

Co-authored-by: Ivan Vakhrushev <i.vakhrushev@banki.ru>
@mfvanek @Evreke
Bump Gradle and dependencies (#180)
c37bb87 
Co-authored-by: Ivan Vakhrushev <mfvanek@gmail.com>
@mfvanek @Evreke
Prepare for the next development iteration (#181)
a5a8166 
Co-authored-by: Ivan Vakhrushev <mfvanek@gmail.com>
@Evreke
Background primary determiner #121
cab39ab 
change: resolve comments
Signed-off-by: Antipin Alexey <evreke@gmail.com>
@Evreke Evreke force-pushed the feature/background-primary-determiner branch from f040c47 to cab39ab Compare September 30, 2022 10:21
@Evreke Evreke marked this pull request as ready for review September 30, 2022 10:26
@Evreke
Copy link
Collaborator Author

Evreke commented Sep 30, 2022

I can't understand why codecov shows me some lines partially covered. @mfvanek please, need help here.

@mfvanek
Copy link
Owner

mfvanek commented Sep 30, 2022

97.1% Coverage

It's here
Screenshot 2022-09-30 at 17 38 48

@mfvanek
Copy link
Owner

mfvanek commented Sep 30, 2022

@Evreke you should implement a test when primary host doesn't change within a delay period

@mfvanek
Copy link
Owner

mfvanek commented Sep 30, 2022 

FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':jacocoTestCoverageVerification'.
> Rule violated for bundle pg-index-health: branches covered ratio is 0.9, but expected minimum is 1.0

@Evreke
Copy link
Collaborator Author

Evreke commented Oct 4, 2022 •
edited
Loading

@Evreke you should implement a test when primary host doesn't change within a delay period

It turns out that it's not easy to test such case without reconfiguring cluster. Definitely, we should be able to run a separate cluster for each test.

@mfvanek should I create a issue for this or I can implement such feature right here?

@mfvanek
Copy link
Owner

mfvanek commented Oct 6, 2022

@Evreke
This PR cannot be merged without all checks passed. So you definitely have to test your code in all ways. Right here

@Evreke
Background primary determiner #121
831973f 
change: resolve comments
Signed-off-by: Antipin Alexey <evreke@gmail.com>
@Evreke Evreke force-pushed the feature/background-primary-determiner branch from e7ddecb to 831973f Compare October 10, 2022 15:20
@Evreke Evreke requested a review from mfvanek October 10, 2022 15:24
Evreke and others added 4 commits October 10, 2022 18:26
@Evreke
Merge branch 'master' into feature/background-primary-determiner
1a22993
1) Spotbugs exclusions moved to config. We do not dirty the code with…
145d99d 
… SuppressFBWarnings.

2) Added javadoc to HighAvailabilityPgConnectionImpl
3) Refactored construction methods in HighAvailabilityPgConnectionImpl.
4) Mocked tests moved to HighAvailabilityPgConnectionUnitTest with "fast" tag.
5) ClusterAwareTestBase renamed to PgConnectionAwareCluster
6) Implemented AutoCloseable for all Testcontainers wrappers.
6) Migrated tests to try-with-resources.
Fixed build on MacOS
280dafc
Fixed code smell
7d3c80c
mfvanek
mfvanek previously approved these changes Oct 16, 2022
View reviewed changes
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

@mfvanek mfvanek merged commit 744975c into master Oct 16, 2022
@mfvanek mfvanek deleted the feature/background-primary-determiner branch October 16, 2022 09:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mfvanek mfvanek mfvanek approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Evreke @mfvanek