Skip to content

Conversation

@zhulipeng
Copy link
Contributor

@zhulipeng zhulipeng commented Mar 15, 2019

What changes were proposed in this pull request?

This PR aims to add a JDBC integration test for MsSql server.

How was this patch tested?

./build/mvn clean install -DskipTests
./build/mvn test -Pdocker-integration-tests -pl :spark-docker-integration-tests_2.12 \
-Dtest=none -DwildcardSuites=org.apache.spark.sql.jdbc.MsSqlServerIntegrationSuite

@zhulipeng zhulipeng changed the title Add docker integration test for MsSql server [Spark-27168]Add docker integration test for MsSql server Mar 15, 2019
@srowen
Copy link
Member

srowen commented Mar 15, 2019

I am not sure how much SQL Server is intended to be supported or how much overhead this testing adds. I'm not against it.

@zhulipeng
Copy link
Contributor Author

@srowen These docker integration tests are in external directory, they need user to manually kickoff in locally, I think it will not affects the build process overheads.

@dongjoon-hyun dongjoon-hyun changed the title [Spark-27168]Add docker integration test for MsSql server [SPARK-27168][TEST] Add docker integration test for MsSql server Mar 18, 2019
@dongjoon-hyun
Copy link
Member

ok to test

Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

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

@lipzhu . This test suite should be verified manually. And, I cannot pass the tests with AS-IS PR code. @srowen . Was this okay in your environment? I want to be sure about that.

@dongjoon-hyun
Copy link
Member

dongjoon-hyun commented Mar 18, 2019

Oh, I got it. We need the previous 8ee09f2 on Friday. Let me rebase this PR and test again.
Hmm. After rebasing and building cleanly, I'm still facing the same failure.

@zhulipeng
Copy link
Contributor Author

Oh, I got it. We need the previous 8ee09f2 on Friday. Let me rebase this PR and test again.
Hmm. After rebasing and building cleanly, I'm still facing the same failure.

@dongjoon-hyun I just double confirm in local, the MsSqlServerIntegrationSuite can success.
Can you paste the error log here?
image

BTW, The memory setting have some gaps between tag latest and 2017-GA-ubuntu
Below is the memory requirement for docker container.
image

@SparkQA
Copy link

SparkQA commented Mar 18, 2019

Test build #103595 has finished for PR 24099 at commit 7a844e1.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Mar 18, 2019

Test build #103598 has finished for PR 24099 at commit 1b4bd43.

  • This patch fails due to an unknown error code, -9.
  • This patch merges cleanly.
  • This patch adds no public classes.

@zhulipeng
Copy link
Contributor Author

zhulipeng commented Mar 18, 2019

reset this please.

@dongjoon-hyun
Copy link
Member

dongjoon-hyun commented Mar 18, 2019

Interesting. The error log was #24099 (comment) .
And, my Docker machine configuration is 10GB RAM / 8 Core / DockerDesktop Community(2.0.0.3) / Mac.

I'll try again since this is updated. Thanks, @lipzhu .

@dongjoon-hyun
Copy link
Member

dongjoon-hyun commented Mar 18, 2019

@lipzhu . If you clear your maven repository and run ./build/mvn test -Pdocker-integration-tests -pl :spark-docker-integration-tests_2.12, you can see the failure.

For Apache Spark contribution, I'd like to recommend you the followings.

  • To get a correct review, the PR itself should be a complete code, but this is not.
    Although Jenkins builds PRs after rebasing, please don't assume that the reviewer to rebase your PR to the master branch. Technically, we need 8ee09f2 in this PR, isn't it?

  • Before testing locally, please remove your local maven repository. It's an essential step for sanity. Docker integration suite uses the snapshot artifacts in the following location. However, your previous PR is not published there yet.

Due to (1) and (2), only you pass the test locally.

Technically, this PR needs three following steps. Otherwise, it fails.

  1. Rebased this PR to bring the previous patch.
  2. Do mvn clean -DskipTests install.
  3. Run build/mvn test -Pdocker-integration-tests -pl :spark-docker-integration-tests_2.12.

Lastly, please update How was this patch tested? section in this PR description. Otherwise, we need to wait until the snapshot artifacts are ready.

@zhulipeng
Copy link
Contributor Author

Got it, thanks for your comments and review. @dongjoon-hyun

@dongjoon-hyun
Copy link
Member

Thank you for updating, @lipzhu ! I'll review again in a few hours.

@dongjoon-hyun
Copy link
Member

dongjoon-hyun commented Mar 19, 2019

Hi, @lipzhu .
I updated the PR description and made a refactoring PR to you. Could you review and merge that please? (I know this PR follows the existing style, but the existing style is a little old for Spark 3.0.)

@dongjoon-hyun
Copy link
Member

dongjoon-hyun commented Mar 19, 2019

Thank you for review and applying.

@dongjoon-hyun dongjoon-hyun changed the title [SPARK-27168][TEST] Add docker integration test for MsSql server [SPARK-27168][SQL][TEST] Add docker integration test for MsSql server Mar 19, 2019
Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

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

+1, LGTM (Pending Jenkins).
Thank you for doing this.

@SparkQA
Copy link

SparkQA commented Mar 19, 2019

Test build #103655 has finished for PR 24099 at commit 65f6fe8.

  • This patch fails due to an unknown error code, -9.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
  • assert(types(1).equals(\"class java.lang.Integer\"))
  • assert(types(2).equals(\"class java.lang.Integer\"))
  • assert(types(3).equals(\"class java.lang.Integer\"))
  • assert(types(4).equals(\"class java.lang.Long\"))
  • assert(types(5).equals(\"class java.lang.Double\"))
  • assert(types(6).equals(\"class java.lang.Double\"))
  • assert(types(7).equals(\"class java.lang.Double\"))
  • assert(types(8).equals(\"class java.math.BigDecimal\"))
  • assert(types(11).equals(\"class java.math.BigDecimal\"))

@SparkQA
Copy link

SparkQA commented Mar 19, 2019

Test build #103649 has finished for PR 24099 at commit bc6ed59.

  • This patch fails due to an unknown error code, -9.
  • This patch merges cleanly.
  • This patch adds no public classes.

@dilipbiswal
Copy link
Contributor

retest this please

@SparkQA
Copy link

SparkQA commented Mar 19, 2019

Test build #103665 has finished for PR 24099 at commit 65f6fe8.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
  • assert(types(1).equals(\"class java.lang.Integer\"))
  • assert(types(2).equals(\"class java.lang.Integer\"))
  • assert(types(3).equals(\"class java.lang.Integer\"))
  • assert(types(4).equals(\"class java.lang.Long\"))
  • assert(types(5).equals(\"class java.lang.Double\"))
  • assert(types(6).equals(\"class java.lang.Double\"))
  • assert(types(7).equals(\"class java.lang.Double\"))
  • assert(types(8).equals(\"class java.math.BigDecimal\"))
  • assert(types(11).equals(\"class java.math.BigDecimal\"))

@dongjoon-hyun
Copy link
Member

Thank you, @lipzhu , @srowen , @dilipbiswal .
Merged to master.

shivsood pushed a commit to shivsood/spark that referenced this pull request Jul 24, 2019
## What changes were proposed in this pull request?

This PR aims to add a JDBC integration test for MsSql server.

## How was this patch tested?

```
./build/mvn clean install -DskipTests
./build/mvn test -Pdocker-integration-tests -pl :spark-docker-integration-tests_2.12 \
-Dtest=none -DwildcardSuites=org.apache.spark.sql.jdbc.MsSqlServerIntegrationSuite
```

Closes apache#24099 from lipzhu/SPARK-27168.

Lead-authored-by: Zhu, Lipeng <[email protected]>
Co-authored-by: Dongjoon Hyun <[email protected]>
Co-authored-by: Lipeng Zhu <[email protected]>
Signed-off-by: Dongjoon Hyun <[email protected]>
@dongjoon-hyun
Copy link
Member

Hi, All (and @gatorsmile )
I'll backport this test patch from master to branch-2.4 in order to support Apache Spark 2.4.x LTS branch in a more robust way.

Since this integration test suite is not tested on Jenkins, I cherry-picked and tested locally.
(Backporting was a clean cherry-pick.)

dongjoon-hyun added a commit that referenced this pull request Jul 24, 2019
## What changes were proposed in this pull request?

This PR aims to add a JDBC integration test for MsSql server.

## How was this patch tested?

```
./build/mvn clean install -DskipTests
./build/mvn test -Pdocker-integration-tests -pl :spark-docker-integration-tests_2.12 \
-Dtest=none -DwildcardSuites=org.apache.spark.sql.jdbc.MsSqlServerIntegrationSuite
```

Closes #24099 from lipzhu/SPARK-27168.

Lead-authored-by: Zhu, Lipeng <[email protected]>
Co-authored-by: Dongjoon Hyun <[email protected]>
Co-authored-by: Lipeng Zhu <[email protected]>
Signed-off-by: Dongjoon Hyun <[email protected]>
rluta pushed a commit to rluta/spark that referenced this pull request Sep 17, 2019
## What changes were proposed in this pull request?

This PR aims to add a JDBC integration test for MsSql server.

## How was this patch tested?

```
./build/mvn clean install -DskipTests
./build/mvn test -Pdocker-integration-tests -pl :spark-docker-integration-tests_2.12 \
-Dtest=none -DwildcardSuites=org.apache.spark.sql.jdbc.MsSqlServerIntegrationSuite
```

Closes apache#24099 from lipzhu/SPARK-27168.

Lead-authored-by: Zhu, Lipeng <[email protected]>
Co-authored-by: Dongjoon Hyun <[email protected]>
Co-authored-by: Lipeng Zhu <[email protected]>
Signed-off-by: Dongjoon Hyun <[email protected]>
kai-chi pushed a commit to kai-chi/spark that referenced this pull request Sep 26, 2019
## What changes were proposed in this pull request?

This PR aims to add a JDBC integration test for MsSql server.

## How was this patch tested?

```
./build/mvn clean install -DskipTests
./build/mvn test -Pdocker-integration-tests -pl :spark-docker-integration-tests_2.12 \
-Dtest=none -DwildcardSuites=org.apache.spark.sql.jdbc.MsSqlServerIntegrationSuite
```

Closes apache#24099 from lipzhu/SPARK-27168.

Lead-authored-by: Zhu, Lipeng <[email protected]>
Co-authored-by: Dongjoon Hyun <[email protected]>
Co-authored-by: Lipeng Zhu <[email protected]>
Signed-off-by: Dongjoon Hyun <[email protected]>
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.

5 participants