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

Add SpannerHealthIndicator and AutoConfiguration #643

Merged
merged 9 commits into from
Nov 9, 2021
Merged

Add SpannerHealthIndicator and AutoConfiguration #643

merged 9 commits into from
Nov 9, 2021

Conversation

ikeyat
Copy link
Contributor

@ikeyat ikeyat commented Oct 5, 2021 •
edited

Fixes #640

Following tasks have been completed. Please review.

Don't merge. Work in progress:

  • Test classes are not implemented
  • The documentation is not updated

@ikeyat ikeyat changed the title WIP: Add SpannerHealthIndicator and AutoConfiguration Add SpannerHealthIndicator and AutoConfiguration Oct 9, 2021
@ikeyat
Copy link
Contributor Author

ikeyat commented Oct 9, 2021 •
edited

I fixed and added tests. Please review.

@ikeyat
Copy link
Contributor Author

ikeyat commented Oct 11, 2021

Thanks for the execution of checks.
I got a checkstyle error about import usage. I'll fix it soon.

@suztomo suztomo requested a review from elefeint October 11, 2021 15:17
@suztomo
Copy link
Contributor

suztomo commented Oct 11, 2021

I see the tests passed but "Don't merge. Work in progress:" in the pull request description.

@ikeyat
Copy link
Contributor Author

ikeyat commented Oct 11, 2021

@suztomo
Thanks. I forgot to update it.
I updated just now. Please review and merge if acceptable.

@ikeyat
Copy link
Contributor Author

ikeyat commented Oct 12, 2021

Actually documentation is not completed.
I'll write it after source review is completed.

@mpeddada1
Copy link
Contributor

Hey @ikeyat, we still need a few more days to review your PR. Thank you for your patience! We appreciate your contribution.

elefeint
elefeint suggested changes Oct 31, 2021
View reviewed changes
Copy link
Contributor

@elefeint elefeint left a comment

Choose a reason for hiding this comment

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

Thank you, and sorry for the delay.
Please add @since 2.0.6 tags to new classes, and there is one more inline comment. Otherwise looks very good.

...c/main/java/com/google/cloud/spring/autoconfigure/spanner/health/SpannerHealthIndicator.java Outdated

@Override
protected void doHealthCheck(Builder builder) throws Exception {
ResultSet resultSet = spannerTemplate.executeQuery(Statement.of(validationQuery), null);
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this will be the same Statement every time, it would be better to create the Statement in constructor and store that as a field instead of String validationQuery;.

...t/java/com/google/cloud/spring/autoconfigure/spanner/health/SpannerHealthIndicatorTests.java Outdated

@Test
public void testdoHealthCheckUp() throws Exception {
SpannerHealthIndicator SpannerHealthIndicator = new SpannerHealthIndicator(spannerTemplate, QUERY);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
SpannerHealthIndicator SpannerHealthIndicator = new SpannerHealthIndicator(spannerTemplate, QUERY);
SpannerHealthIndicator spannerHealthIndicator = new SpannerHealthIndicator(spannerTemplate, QUERY);

...t/java/com/google/cloud/spring/autoconfigure/spanner/health/SpannerHealthIndicatorTests.java Outdated

Health.Builder builder = new Health.Builder();

SpannerHealthIndicator.doHealthCheck(builder);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
SpannerHealthIndicator.doHealthCheck(builder);
spannerHealthIndicator.doHealthCheck(builder);

...t/java/com/google/cloud/spring/autoconfigure/spanner/health/SpannerHealthIndicatorTests.java Outdated

@Test(expected = Exception.class)
public void testdoHealthCheckDownSpannerTemplate() throws Exception {
SpannerHealthIndicator SpannerHealthIndicator = new SpannerHealthIndicator(spannerTemplate, QUERY);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
SpannerHealthIndicator SpannerHealthIndicator = new SpannerHealthIndicator(spannerTemplate, QUERY);
SpannerHealthIndicator spannerHealthIndicator = new SpannerHealthIndicator(spannerTemplate, QUERY);

...t/java/com/google/cloud/spring/autoconfigure/spanner/health/SpannerHealthIndicatorTests.java Outdated

Health.Builder builder = new Health.Builder();

SpannerHealthIndicator.doHealthCheck(builder);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
SpannerHealthIndicator.doHealthCheck(builder);
spannerHealthIndicator.doHealthCheck(builder);

...t/java/com/google/cloud/spring/autoconfigure/spanner/health/SpannerHealthIndicatorTests.java Outdated

@Test
public void testHealthy() {
SpannerHealthIndicator SpannerHealthIndicator = new SpannerHealthIndicator(spannerTemplate, QUERY);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
SpannerHealthIndicator SpannerHealthIndicator = new SpannerHealthIndicator(spannerTemplate, QUERY);
spannerHealthIndicator SpannerHealthIndicator = new SpannerHealthIndicator(spannerTemplate, QUERY);

...t/java/com/google/cloud/spring/autoconfigure/spanner/health/SpannerHealthIndicatorTests.java Outdated

@Test
public void testUnhealthySpannerTemplate() {
SpannerHealthIndicator SpannerHealthIndicator = new SpannerHealthIndicator(spannerTemplate, QUERY);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
SpannerHealthIndicator SpannerHealthIndicator = new SpannerHealthIndicator(spannerTemplate, QUERY);
SpannerHealthIndicator spannerHealthIndicator = new SpannerHealthIndicator(spannerTemplate, QUERY);

...t/java/com/google/cloud/spring/autoconfigure/spanner/health/SpannerHealthIndicatorTests.java Outdated

when(spannerTemplate.executeQuery(any(), any())).thenThrow(new RuntimeException("Cloud Spanner is down!!!"));

assertThat(SpannerHealthIndicator.health().getStatus()).isEqualTo(Status.DOWN);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
assertThat(SpannerHealthIndicator.health().getStatus()).isEqualTo(Status.DOWN);
assertThat(spannerHealthIndicator.health().getStatus()).isEqualTo(Status.DOWN);

...t/java/com/google/cloud/spring/autoconfigure/spanner/health/SpannerHealthIndicatorTests.java Outdated

@Test
public void testUnhealthyResultSet() {
SpannerHealthIndicator SpannerHealthIndicator = new SpannerHealthIndicator(spannerTemplate, QUERY);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
SpannerHealthIndicator SpannerHealthIndicator = new SpannerHealthIndicator(spannerTemplate, QUERY);
SpannerHealthIndicator spannerHealthIndicator = new SpannerHealthIndicator(spannerTemplate, QUERY);

...t/java/com/google/cloud/spring/autoconfigure/spanner/health/SpannerHealthIndicatorTests.java Outdated
when(spannerTemplate.executeQuery(any(), any())).thenReturn(resultSet);
when(resultSet.next()).thenThrow(new RuntimeException("Cloud Spanner is down!!!"));

assertThat(SpannerHealthIndicator.health().getStatus()).isEqualTo(Status.DOWN);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
assertThat(SpannerHealthIndicator.health().getStatus()).isEqualTo(Status.DOWN);
assertThat(spannerHealthIndicator.health().getStatus()).isEqualTo(Status.DOWN);

@ikeyat
Copy link
Contributor Author

ikeyat commented Nov 1, 2021

@elefeint
Thanks for your review. I fixed them.

elefeint
elefeint approved these changes Nov 5, 2021
View reviewed changes
Copy link
Contributor

@elefeint elefeint left a comment

Choose a reason for hiding this comment

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

This looks great, thank you! Could you add documentation to spanner.adoc? Especially around turning the indicator off.

@ikeyat
Copy link
Contributor Author

ikeyat commented Nov 8, 2021

@elefeint
I've updated the adoc!

@sonarcloud
Copy link

sonarcloud bot commented Nov 8, 2021

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

@elefeint elefeint merged commit 2c194a5 into GoogleCloudPlatform:main Nov 9, 2021
@elefeint
Copy link
Contributor

elefeint commented Nov 9, 2021

@ikeyat Thank you!

@elefeint elefeint mentioned this pull request Jan 21, 2022
Closed
kateryna216 added a commit to kateryna216/spring-cloud-gcp that referenced this pull request Oct 20, 2022
@kateryna216
Add SpannerHealthIndicator and AutoConfiguration (GoogleCloudPlatform…
39f3d01 
…#643)

Add SpannerHealthIndicator and AutoConfiguration.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@elefeint elefeint elefeint 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.

Health Indicator for Cloud Spanner
4 participants
@ikeyat @suztomo @mpeddada1 @elefeint