Skip to content

Support for SAP Hana Database #14513

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
Closed
wants to merge 4 commits into from
Closed

Support for SAP Hana Database #14513

wants to merge 4 commits into from

Conversation

MichaelSp
Copy link
Contributor

The driver itself can be found here:

<dependency>
    <groupId>com.sap.db.jdbc</groupId>
    <artifactId>ngdbc</artifactId>
</dependency>

@pivotal-issuemaster
Copy link

@MichaelSp Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

@pivotal-issuemaster
Copy link

@MichaelSp Thank you for signing the Contributor License Agreement!

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Sep 19, 2018
snicoll
snicoll reviewed Sep 19, 2018
View reviewed changes
Copy link
Member

@snicoll snicoll left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. I've added a comment.

spring-boot-project/spring-boot/src/main/java/org/springframework/boot/jdbc/DatabaseDriver.java Outdated
@@ -16,14 +16,14 @@

package org.springframework.boot.jdbc;

import org.springframework.util.Assert;
Copy link
Member

Choose a reason for hiding this comment

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

This layout change seems unrelated. Could you please revert that part?

@snicoll snicoll added the status: waiting-for-feedback We need additional information before we can continue label Sep 19, 2018
snicoll
snicoll reviewed Sep 19, 2018
View reviewed changes
spring-boot-project/spring-boot/src/main/java/org/springframework/boot/jdbc/DatabaseDriver.java Outdated
/**
* SAP - SAP Hana Database - HDB.
*/
HDB("HDB", "com.sap.db.jdbc.Driver", "com.sap.db.jdbcext.XADataSourceSAP",
Copy link
Member

Choose a reason for hiding this comment

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

Given the package space, is there any concrete reason why this can't be named SAP? If the JDBC id is sap I think it should be fine naming it that way. Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The driver itself has this:

    public static String getDatabaseName() {
        return "HDB";
    }

    public static String getDriverName() {
        return "HDB";
    }

I don't know the implications, but to me it looks like the name "HDB" aligns better. Open for discussion....

@snicoll snicoll added the for: team-attention An issue we'd like other members of the team to review label Sep 19, 2018
@MichaelSp
Copy link
Contributor Author

MichaelSp commented Sep 19, 2018 •
edited
Loading

arghs...

SAP doesn't allowed redistribution of ngdbc.jar in their TC as mentioned here. So you won't find any legal one online.
Your only option is to include the jar from your local.

https://stackoverflow.com/questions/42431906/ngdbc-jar-equivalent-maven-repository

any idea?

@snicoll
Copy link
Member

snicoll commented Sep 19, 2018 •
edited
Loading

You shouldn't add a reference to the driver as we're not offering dependency management for it. It would be nice if you had a single proposal rather than opening a PR and then amending things after the fact.

Please remove the driver in the pom, it is not needed.

@MichaelSp
Copy link
Contributor Author

Ok, sorry. I thought the change is straight forward...

@MichaelSp
overwrite "getUrlPrefixes"
e4c75f3 
Signed-off-by: Michael Sprauer <Michael.Sprauer@sap.com>
@snicoll snicoll removed the for: team-attention An issue we'd like other members of the team to review label Sep 19, 2018
@snicoll
Copy link
Member

snicoll commented Sep 19, 2018

@MichaelSp we've just discussed this and we think SAP is a better name for the enum. Can you please restore your proposal to use that name?

@snicoll snicoll added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Sep 19, 2018
@snicoll snicoll added this to the 2.1.x milestone Sep 19, 2018
@MichaelSp
renamed HDB->SAP
8971c49 
Signed-off-by: Michael Sprauer <Michael.Sprauer@sap.com>
@snicoll snicoll self-assigned this Sep 19, 2018
@snicoll snicoll removed the status: waiting-for-feedback We need additional information before we can continue label Sep 19, 2018
snicoll pushed a commit that referenced this pull request Sep 20, 2018
@MichaelSp @snicoll
Add support for SAP Hana Database
cea95c3 
See gh-14513
@snicoll snicoll closed this in 22e77e0 Sep 20, 2018
snicoll added a commit that referenced this pull request Sep 20, 2018
@snicoll
Merge pull request #14513 from MichaelSp:patch-1
4ad2f1d 
* pr/14513:
  Polish "Add support for SAP Hana Database"
  Add support for SAP Hana Database
@wilkinsona wilkinsona modified the milestones: 2.1.x, 2.1.0.M4 Sep 20, 2018
@breglerj breglerj mentioned this pull request Nov 7, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@snicoll snicoll snicoll approved these changes

Assignees

@snicoll snicoll

Labels
type: enhancement A general enhancement
Projects
None yet
Milestone
2.1.0.M4
Development

Successfully merging this pull request may close these issues.
5 participants
@MichaelSp @pivotal-issuemaster @snicoll @wilkinsona @spring-projects-issues