[BEAM-5729] added database/sql reader/writer #6676

adranwit · 2018-10-12T18:24:43Z

BEAM-5728 Adds database/sql reader/writer in go sdk

Follow this checklist to help us incorporate your contribution quickly and easily:

Format the pull request title like [BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replace BEAM-XXX with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
If this contribution is large, please file an Apache Individual Contributor License Agreement.

It will help us expedite review of your Pull Request if you tag someone (e.g. @username) to look at it.

Post-Commit Tests Status (on master branch)

Lang	Apex	Dataflow	Flink	Gearpump	Samza	Spark
Go	---	---	---	---	---	---
Java
Python	---			---	---	---

adranwit · 2018-10-12T18:49:17Z

passed initial chec

chamikaramj · 2018-11-13T01:01:28Z

R: @lostluck

chamikaramj · 2018-11-13T01:02:15Z

Sorry about the delay. Adding Robert as a reviewer.

Any idea about the test failures ?

lostluck · 2018-11-13T03:18:07Z

The RAT failures are because the license text needs to be at the top of every file.

The go failure appears to be this test: From the full jenkins output:
https://builds.apache.org/job/beam_PreCommit_Go_Commit/1107/consoleFull

06:06:40 === RUN Test_queryRecordMapperProvider
06:06:40 --- PASS: Test_queryRecordMapperProvider (0.00s)
06:06:40 === RUN Test_writerRecordMapperProvider
06:06:40 --- FAIL: Test_writerRecordMapperProvider (0.00s)
06:06:40 panic: runtime error: index out of range [recovered]
06:06:40 panic: runtime error: index out of range
06:06:40
06:06:40 goroutine 6 [running]:
06:06:40 testing.tRunner.func1(0xc4201d61e0)
06:06:40 /home/jenkins/.gradle/go/binary/1.10/go/src/testing/testing.go:742 +0x29d
06:06:40 panic(0x95a4c0, 0xdae210)
06:06:40 /home/jenkins/.gradle/go/binary/1.10/go/src/runtime/panic.go:505 +0x229
06:06:40 github.com/apache/beam/sdks/go/pkg/beam/io/databaseio.newWriterRowMapper.func1(0x9c49e0, 0xc4200aac40, 0x199, 0xc4200aac40, 0x16, 0x0, 0x0, 0x1)
06:06:40 /home/jenkins/jenkins-slave/workspace/beam_PreCommit_Go_Commit/src/sdks/go/.gogradle/project_gopath/src/github.com/apache/beam/sdks/go/pkg/beam/io/databaseio/mapper.go:114 +0x23b
06:06:40 github.com/apache/beam/sdks/go/pkg/beam/io/databaseio.Test_writerRecordMapperProvider(0xc4201d61e0)
06:06:40 /home/jenkins/jenkins-slave/workspace/beam_PreCommit_Go_Commit/src/sdks/go/.gogradle/project_gopath/src/github.com/apache/beam/sdks/go/pkg/beam/io/databaseio/util_test.go:75 +0x239
06:06:40 testing.tRunner(0xc4201d61e0, 0xa4d0a0)
06:06:40 /home/jenkins/.gradle/go/binary/1.10/go/src/testing/testing.go:777 +0xd0
06:06:40 created by testing.(*T).Run
06:06:40 /home/jenkins/.gradle/go/binary/1.10/go/src/testing/testing.go:824 +0x2e0
06:06:40 FAIL github.com/apache/beam/sdks/go/pkg/beam/io/databaseio 0.011s
06:06:40
06:06:40 > Task :beam-sdks-go:test
06:06:40 Test for github.com/apache/beam/sdks/go/pkg/beam/io/databaseio finished, 2 completed, 1 failed

lostluck

I love how complete this CL is. Once we get the tests passing, we should get this in.

There are opportunities to reduce overhead, either in connections to the DB or in reducing work done per element, but that can be done later as needed.

I tried to figure out what's going wrong with the test, but nothing is jumping out at me, it appears the mapping should work as expected. I assume that the test passes locally for you.

sdks/go/pkg/beam/io/databaseio/database.go

sdks/go/pkg/beam/io/databaseio/mapper.go

sdks/go/pkg/beam/io/databaseio/writer.go

sdks/go/pkg/beam/io/databaseio/database.go

lostluck · 2018-11-13T16:47:25Z

Consider editing the title of this PR to be prefixed with [BEAM-5729] so it gets linked to the Jira you filed before. https://issues.apache.org/jira/browse/BEAM-5729

Consider also requesting contributor status to the Beam jira, so we can correctly assign the JIRA to you. Afterall, you did all this work!

lostluck

Thanks for your patience. I have a few small comments, but otherwise LGTM.

sdks/go/pkg/beam/io/databaseio/database.go

lostluck · 2018-11-13T17:37:07Z

Thanks again!
@chamikaramj Please merge.

Edit: Though you could prefix the github PR title with [BEAM-5729], that would be ideal.
[BEAM-5729] added database/sql reader/writer

chamikaramj · 2018-11-13T17:56:26Z

Thanks. @adranwit could you fix up to a single (or few) commits(s) for merging ?

adranwit · 2018-11-13T18:10:23Z

@chamikaramj not sure what the ask is "could you fix up to a single (or few) commit (s)", what exactly needs to be fixed ?

chamikaramj · 2018-11-13T18:25:29Z

Sorry, could you fix up your commits (currently 11) to one or few commits that you would like to appear in Beam commit history as per https://beam.apache.org/contribute/committer-guide/.

adranwit · 2018-11-13T18:38:06Z

@chamikaramj not sure if you can remove existing comments, all that said I have added the extra one summarizing the changes: [BEAM-5729] added database/sql based reader and writer

chamikaramj · 2018-11-13T18:40:36Z

You can use "git rebase" to fixup commits. See https://help.github.com/articles/about-git-rebase/.

chamikaramj · 2018-11-14T02:23:14Z

Looks like commit history got messed up somehow ?

Usually I perform following to fixup all my commits.

(1) git rebase -i HEAD~<number of my commits>
(2) add 'f' in front of everything but the first commit for fixing up.
(3) git commit --amend (to update the commit if needed)
(3) git push -f <my github fork>

chamikaramj · 2018-11-14T17:55:58Z

sdks/python/apache_beam/testing/util_test.py

@@ -38,6 +38,14 @@ def test_assert_that_passes(self):
    with TestPipeline() as p:
      assert_that(p | Create([1, 2, 3]), equal_to([1, 2, 3]))

+  def test_assert_that_passes_order_does_not_matter(self):
+    with TestPipeline() as p:


Are these changes (to util_test.py) intended ? Looks like a merge issue.

Everything else looks good. Thanks.

not sure how to get that removed

chamikaramj · 2018-11-15T00:26:23Z

Thanks. Merged.

adranwit closed this Oct 12, 2018

adranwit reopened this Oct 12, 2018

adranwit closed this Oct 12, 2018

adranwit reopened this Oct 12, 2018

adranwit closed this Oct 12, 2018

adranwit reopened this Oct 12, 2018

lostluck reviewed Nov 13, 2018

View reviewed changes

lostluck approved these changes Nov 13, 2018

View reviewed changes

sdks/go/pkg/beam/io/databaseio/database.go Outdated Show resolved Hide resolved

sdks/go/pkg/beam/io/databaseio/database.go Outdated Show resolved Hide resolved

adranwit changed the title ~~added database/sql reader/writer~~ [BEAM-5729] added database/sql reader/writer Nov 13, 2018

[BEAM-5729] added database/sql based reader and writer

d986807

adranwit force-pushed the master branch from 8375d70 to d986807 Compare November 14, 2018 04:51

chamikaramj reviewed Nov 14, 2018

View reviewed changes

chamikaramj merged commit 285923a into apache:master Nov 15, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BEAM-5729] added database/sql reader/writer #6676

[BEAM-5729] added database/sql reader/writer #6676

adranwit commented Oct 12, 2018 •

edited

Loading

adranwit commented Oct 12, 2018

chamikaramj commented Nov 13, 2018

chamikaramj commented Nov 13, 2018

lostluck commented Nov 13, 2018

lostluck left a comment

lostluck commented Nov 13, 2018

lostluck left a comment

lostluck commented Nov 13, 2018 •

edited

Loading

chamikaramj commented Nov 13, 2018

adranwit commented Nov 13, 2018

chamikaramj commented Nov 13, 2018

adranwit commented Nov 13, 2018

chamikaramj commented Nov 13, 2018

chamikaramj commented Nov 14, 2018 •

edited

Loading

chamikaramj Nov 14, 2018

chamikaramj Nov 14, 2018

adranwit Nov 14, 2018

chamikaramj commented Nov 15, 2018

[BEAM-5729] added database/sql reader/writer #6676

[BEAM-5729] added database/sql reader/writer #6676

Conversation

adranwit commented Oct 12, 2018 • edited Loading

BEAM-5728 Adds database/sql reader/writer in go sdk

Post-Commit Tests Status (on master branch)

adranwit commented Oct 12, 2018

chamikaramj commented Nov 13, 2018

chamikaramj commented Nov 13, 2018

lostluck commented Nov 13, 2018

lostluck left a comment

Choose a reason for hiding this comment

lostluck commented Nov 13, 2018

lostluck left a comment

Choose a reason for hiding this comment

lostluck commented Nov 13, 2018 • edited Loading

chamikaramj commented Nov 13, 2018

adranwit commented Nov 13, 2018

chamikaramj commented Nov 13, 2018

adranwit commented Nov 13, 2018

chamikaramj commented Nov 13, 2018

chamikaramj commented Nov 14, 2018 • edited Loading

chamikaramj Nov 14, 2018

Choose a reason for hiding this comment

chamikaramj Nov 14, 2018

Choose a reason for hiding this comment

adranwit Nov 14, 2018

Choose a reason for hiding this comment

chamikaramj commented Nov 15, 2018

adranwit commented Oct 12, 2018 •

edited

Loading

lostluck commented Nov 13, 2018 •

edited

Loading

chamikaramj commented Nov 14, 2018 •

edited

Loading