HANA database provider #893

mrylov · 2022-11-30T10:28:19Z

Dear Tegola community,

We, the SAP HANA Spatial Team, would like to add HANA support to the Tegola tile server. SAP HANA is an in-memory database with an OGC-compliant spatial engine. A free community edition of SAP HANA is available here.

Content

This pull request includes:

Implementation of the provider (provider and mvtprovider interfaces)
Provider documentation
Unit tests
CI integration using a HANA Cloud instance

Requirements

The HANA provider requires the open-source library go-hdb that is used to connect to a HANA instance. Note, this driver requires at least go v.1.18 and to change some dependencies in the vendor directory.

License

The HANA provider is licensed under the MIT license.

Responsibilities

We promise to keep tracking of new issues/bugs, CI runs, API changes, documentation
improvements related to the HANA provider and make sure that they are fixed/implemented promptly.
There will be a contact person at SAP responsible for the maintenance.

coveralls · 2022-11-30T15:49:58Z

Pull Request Test Coverage Report for Build ffc36ce66-PR-893

801 of 1280 (62.58%) changed or added relevant lines in 6 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+1.6%) to 46.905%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
provider/hana/error.go	0	17	0.0%
provider/hana/util.go	406	584	69.52%
provider/hana/hana.go	363	647	56.11%

Totals
Change from base Build 5e4c8d1cc:	1.6%
Covered Lines:	6546
Relevant Lines:	13956

💛 - Coveralls

ARolek · 2022-11-30T16:57:25Z

@mrylov wow! We sure weren't expecting this PR to show up.

We promise to keep tracking of new issues/bugs, CI runs, API changes, documentation
improvements related to the HANA provider and make sure that they are fixed/implemented promptly.
There will be a contact person at SAP responsible for the maintenance.

When I saw this PR my first thought was "Great! But who's going to help support this functionality?" I'm glad you addressed this. Once we get this through code review let's figure out how to best document this point of contact.

I have not had a chance to review this PR in depth, but my first glance review was looking at which tile providers you implemented. It looks like you're targeting both the normal tile provider and the mvt_provider, which is excellent. I did notice that it looks like there's some commented out code for the NewMVTTileProvider() func that might need to be addressed.

Also a quick request, can you please clean up the commits in this PR? Generally speaking we like commits broken into logical chunks as this helps with code review. Since you're vendoring code in this PR I usually step through the PRs commits not reviewing the commit with the vendored code.

Thanks for the contribution!

mrylov · 2022-12-01T10:55:54Z

Hi @ARolek,
Thank you for your prompt reply. I'm really glad that you are willing to accept our contribution.

As you mentioned above, we did implement both types of providers. However, we did comment some code, as there was an issue in the implementation of the ST_AsMVT function on the HANA side. The issue must have been fixed now.

Regarding your request, I can reorganize the PR in the following commits:

NewTileProvider
- Implementation
- Documentation
- Unit tests
- CI integration
NewMVTTileProvider
- Implementation
- Documentation
- Unit tests

Is this structure sufficient to start the review?

ARolek · 2022-12-01T18:08:34Z

As you mentioned above, we did implement both types of providers. However, we did comment some code, as there was an issue in the implementation of the ST_AsMVT function on the HANA side. The issue must have been fixed now.

Excellent. It would be great to support ST_AsMVT as it's usually more performant and less error prone than tegola's native geo processing.

Regarding your request, I can reorganize the PR in the following commits:

Looks good to me, with one additional request, please have a separate commit for code vendoring. If you want, you could even reduce this to 3 commits: Vendoring, NewTileProvider, NewMVTTileProvider.

Thanks!

mrylov · 2022-12-01T18:30:25Z

Looks good to me, with one additional request, please have a separate commit for code vendoring. If you want, you could even reduce this to 3 commits: Vendoring, NewTileProvider, NewMVTTileProvider.

Thank you @ARolek. What exactly do you mean by Vendoring? Do you mean any changes related to the vendor folder, as well as to the go.mod and go.sum files?

ARolek · 2022-12-01T19:01:44Z

Thank you @ARolek. What exactly do you mean by Vendoring? Do you mean any changes related to the vendor folder, as well as to the go.mod and go.sum files?

Correct. When you run go mod -vendor the database driver is being copied into the vendor folder. This is a lot of code, and it wont get reviewed with the PR. Breaking the code vendoring into a separate commit makes code review easier as I don't need to dig through the driver files.

mrylov · 2022-12-02T13:31:16Z

@ARolek, I've restructured the commits as you requested.
In TestMVTForLayers, there are several TODO comments that need to be addressed once we update our HANA Cloud instance used in CI. We are planning to perform this update within 2-3 weeks.

ARolek · 2022-12-02T22:36:31Z

In TestMVTForLayers, there are several TODO comments that need to be addressed once we update our HANA Cloud instance used in CI. We are planning to perform this update within 2-3 weeks.

Sounds good! we can review the second commit for now. Enjoy the weekend.

mrylov · 2023-01-16T14:19:43Z

@ARolek, I've adjusted some tests in TestMVTForLayers as we updated our HANA instance.

ARolek · 2023-01-20T18:40:13Z

@mrylov

I've adjusted some tests in TestMVTForLayers as we updated our HANA instance.

Excellent! I still need to give this a review. Will do so soon. Can you please rebase against master? It looks like this branch has several conflicts.

mrylov · 2023-01-25T10:35:39Z

@ARolek, I'm looking forward. I've rebased the commits.

mrylov · 2023-03-09T11:14:42Z

Hi @ARolek,
the PR is waiting for a review since end of November. Could you please tell me when you have time to review it?
Thank you.

ARolek

@mrylov apologies for the delay and thanks for the bump. Your PR has been sitting in my inbox since it was originally submitted, life has just been a bit crazy.

I just gave this a review and honestly it's excellent. You blended with the codebase really well, you have applicable tests, flushed out READMEs and clean, organized commits. I don't have any significant comments.

@gdey is going to give this a review too, but I don't have any blockers at this time.

When it comes to supporting this feature, where should we direct anyone inquiring for help on this particular provider?

Thanks for the contribution (and patience)!

provider/hana/register.go

provider/hana/hana.go

mrylov · 2023-03-10T07:55:17Z

Superb! Thank you very much @ARolek.

I will be responsible for supporting the HANA provider, so you can refer anyone to me if there are any questions or issues
concerning this provider.

ARolek · 2023-03-10T16:09:11Z

I will be responsible for supporting the HANA provider, so you can refer anyone to me if there are any questions or issues concerning this provider.

Excellent. I just approved the CI to run on this PR, and the last thing we need is a review from @gdey. I have been in touch with him and it should happen shortly.

gdey

LGTM

This is great! Thank you for the wonderful job!

One quick thing: There looks to be a username and password in the test, if this is true, let's move those creds to GitHub Secrets; if not just let me know, and I'll approve the PR.

Second:
Could you add yourself as the Owner of this code base?
Modify the file: .github/CODEOWNERS and append the following to the end of the file:

#maintainer of HANA database provider
/provider/hana @mrylov

This will ensure that you are included anytime someone touches this path.

Thank you!

.github/workflows/on_pr_push.yml

gdey · 2023-03-14T21:43:37Z

@ARolek Do we already have a PR that updates the UI? The failing test here is because of that. I really don't think that update should be part of this PR.

mrylov · 2023-03-15T06:57:01Z

@gdey Thank you very much for the prompt review.

I've added the code maintainer as you suggested in 63a67b0.

gdey · 2023-03-15T07:01:54Z

LGTM.
Okay will approve it (looks like iOS Github does not let me do code reviews), once i can get to my computer.

gdey

LGTM

gdey · 2023-03-20T17:05:06Z

@mrylov Can we have two things? So, I can merge this in.

rebase it to the latest version of master
squash it down to one commit.

Thank you!

mrylov · 2023-03-21T08:58:42Z

Can we have two things? So, I can merge this in.

@gdey, I've done as you said. Many thanks.

gdey

LGTM, I will deploy once CI is good. (Not counting any UI issues related to #914

gdey · 2023-03-24T13:57:40Z

We will merge this in after #916; which fixes the UI issues the CI on this is breaking on.

@mrylov

As per @mrylov comment here: go-spatial#893 the connection string is exposed on purpose.

@mrylov

As per @mrylov comment here: #893 the connection string is exposed on purpose.

mrylov requested review from gdey and ARolek as code owners November 30, 2022 10:28

mrylov force-pushed the hana_provider_pr branch from d3a55f0 to 12fb39c Compare December 2, 2022 13:22

mrylov force-pushed the hana_provider_pr branch from 12fb39c to 67b9be1 Compare January 16, 2023 14:09

mrylov force-pushed the hana_provider_pr branch from 67b9be1 to 9d0b52e Compare January 25, 2023 10:32

ARolek approved these changes Mar 10, 2023

View reviewed changes

provider/hana/register.go Outdated Show resolved Hide resolved

provider/hana/hana.go Outdated Show resolved Hide resolved

gdey requested changes Mar 14, 2023

View reviewed changes

.github/workflows/on_pr_push.yml Show resolved Hide resolved

gdey approved these changes Mar 17, 2023

View reviewed changes

mrylov force-pushed the hana_provider_pr branch from 63a67b0 to 3f4852f Compare March 21, 2023 08:55

gdey approved these changes Mar 24, 2023

View reviewed changes

iwpnd mentioned this pull request Mar 24, 2023

fix: prometheus with multiple providers #915

Merged

Add HANA tile providers

f56b286

gdey force-pushed the hana_provider_pr branch from 3f4852f to f56b286 Compare March 24, 2023 17:18

gdey merged commit f56b286 into go-spatial:master Mar 24, 2023

gdey mentioned this pull request Mar 28, 2023

ci: rotate connection string #919

Closed

iwpnd added a commit to iwpnd/tegola that referenced this pull request Mar 29, 2023

chore: comment hana connection string

6c620e3

As per @mrylov comment here: go-spatial#893 the connection string is exposed on purpose.

iwpnd mentioned this pull request Mar 29, 2023

chore: comment hana connection string #920

Merged

ARolek pushed a commit that referenced this pull request Mar 29, 2023

chore: comment hana connection string (#920)

62bfffe

As per @mrylov comment here: #893 the connection string is exposed on purpose.

mrylov deleted the hana_provider_pr branch May 26, 2023 08:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HANA database provider #893

HANA database provider #893

mrylov commented Nov 30, 2022

coveralls commented Nov 30, 2022 •

edited

ARolek commented Nov 30, 2022

mrylov commented Dec 1, 2022

ARolek commented Dec 1, 2022

mrylov commented Dec 1, 2022 •

edited

ARolek commented Dec 1, 2022

mrylov commented Dec 2, 2022

ARolek commented Dec 2, 2022

mrylov commented Jan 16, 2023

ARolek commented Jan 20, 2023

mrylov commented Jan 25, 2023

mrylov commented Mar 9, 2023

ARolek left a comment

mrylov commented Mar 10, 2023

ARolek commented Mar 10, 2023

gdey left a comment

gdey commented Mar 14, 2023

mrylov commented Mar 15, 2023

gdey commented Mar 15, 2023

gdey left a comment

gdey commented Mar 20, 2023 •

edited

mrylov commented Mar 21, 2023 •

edited

gdey left a comment

gdey commented Mar 24, 2023

HANA database provider #893

HANA database provider #893

Conversation

mrylov commented Nov 30, 2022

Content

Requirements

License

Responsibilities

coveralls commented Nov 30, 2022 • edited

Pull Request Test Coverage Report for Build ffc36ce66-PR-893

💛 - Coveralls

ARolek commented Nov 30, 2022

mrylov commented Dec 1, 2022

ARolek commented Dec 1, 2022

mrylov commented Dec 1, 2022 • edited

ARolek commented Dec 1, 2022

mrylov commented Dec 2, 2022

ARolek commented Dec 2, 2022

mrylov commented Jan 16, 2023

ARolek commented Jan 20, 2023

mrylov commented Jan 25, 2023

mrylov commented Mar 9, 2023

ARolek left a comment

Choose a reason for hiding this comment

mrylov commented Mar 10, 2023

ARolek commented Mar 10, 2023

gdey left a comment

Choose a reason for hiding this comment

gdey commented Mar 14, 2023

mrylov commented Mar 15, 2023

gdey commented Mar 15, 2023

gdey left a comment

Choose a reason for hiding this comment

gdey commented Mar 20, 2023 • edited

mrylov commented Mar 21, 2023 • edited

gdey left a comment

Choose a reason for hiding this comment

gdey commented Mar 24, 2023

coveralls commented Nov 30, 2022 •

edited

mrylov commented Dec 1, 2022 •

edited

gdey commented Mar 20, 2023 •

edited

mrylov commented Mar 21, 2023 •

edited