Adding batch wrapper on query builder #239

mariosttass · 2022-07-22T12:40:10Z

Hello,

In the following PR we are adding a Batch wrapper on gocqlx library for binding the object instead of using named columns.

Please have a look and let me know for any questions.

Thanks !

tehsphinx · 2022-07-22T12:44:21Z

In case you are not interested in adding this to the library, let us know. Then we'll keep it internally. (which would be harder because of missing access to private gocqlx functionality)

tehsphinx · 2022-07-28T12:46:19Z

@mmatczuk: Any update for us? What do you think about this MR?

tehsphinx

Just had another closer look at the MR.

We should try to get rid of changes to go.mod.
Also wondering about the commit history. Would be nice to have a clean rebase on top of scylladb/gocqlx:master.

go.mod

tehsphinx · 2022-07-28T13:01:40Z

go.mod

 	github.com/google/go-cmp v0.5.4
+	github.com/maraino/go-mock v0.0.0-20180321183845-4c74c434cd3a


If possible also this extra dependency: github.com/maraino/go-mock

This dependency is added because I am using a the go-mock library to mock the session on unit tests

If I get rid of it then I am not sure about the way to mock the session on unit tests.

I removed the tests as they were not really testing anything anyway.

All parts that are used are tested in other unit tests already. We are not really adding something worth testing.

tehsphinx

LGTM

tehsphinx · 2022-10-19T18:48:04Z

@tzach can you have a look? (this is the PR I mentioned on P99 Conference)

mmatczuk · 2022-10-19T19:36:47Z

Can you squash your commits to one and add a test please.

tehsphinx · 2022-10-20T07:31:59Z

Thx for taking a look @mmatczuk!

Can you give us a pointer on how to test this?

We had some tests but they basically mocked everything away not really testing anything in the end. So I removed them. Also note, that we are only using existing functionality that has been tested already. gocql.Batch
(incl. *batch.Query method) is tested in the underlying repository, Queryx and bindStructArgs is tested in query_test.go.

tehsphinx · 2022-10-20T16:41:30Z

Hey @mmatczuk, we have rebased and squashed the commits 🙂

mmatczuk · 2022-10-21T08:21:28Z

Ok would be nice to add some docs so that the users know what it does and why.
I'd ask again to add that to a test. Feel free to copy existing test and replace batch handling with your code.

tehsphinx · 2022-11-14T18:21:13Z

@mmatczuk Sry for the wait. I added some tests now. Also realized the execution functions for batching needed an overwrite to complete the user experience.

Let me know if there is anything else to change 🙂

tehsphinx · 2022-11-30T09:52:53Z

@tzach: can you plan this in to be checked?

tzach · 2022-11-30T10:11:57Z

@avelanarius can you please advise?

tehsphinx · 2023-01-04T13:24:36Z

@tzach Happy new year 🥳!

Can I draw your attention to this once more? Currently we are using this change with a go.mod replace statement. That is not ideal especially since more one more of our microservices need to do that replace.

I'll give this another 2 to 3 weeks and then I'm going to use our clone permanently, changing it's project path so we can import it from our clone without a replace statement.

kaatinga

added a nit

batchx.go

tehsphinx · 2023-02-21T12:19:22Z

@tzach: on the last scylla conference you said, that PRs are always welcome. Seeing all the PRs here waiting for months without any attention paints a very different picture. 😞 😞

mykaul · 2023-02-21T15:08:24Z

@tzach: on the last scylla conference you said, that PRs are always welcome. Seeing all the PRs here waiting for months without any attention paints a very different picture. disappointed disappointed

You are right and we should strive to do a better job here - we are slowly going over the backlog and trying to get stuff in. We wish to keep high quality bar so trying not to blindly merge stuff without fully reviewing the changes. That takes time and resources.

zimnx · 2023-02-22T13:30:42Z

batchx.go

+}
+
+// NewBatch creates a new batch operation using defaults defined in the cluster.
+func (s *Session) NewBatch(typ gocql.BatchType) *Batch {


Suggested change

func (s *Session) NewBatch(typ gocql.BatchType) *Batch {

func (s *Session) NewBatch(bt gocql.BatchType) *Batch {

zimnx · 2023-02-22T13:30:57Z

batchx.go

+// BindStruct binds query named parameters to values from arg using mapper. If
+// value cannot be found error is reported.
+func (b *Batch) BindStruct(qry *Queryx, arg interface{}) error {
+	arglist, err := qry.bindStructArgs(arg, nil)


Suggested change

arglist, err := qry.bindStructArgs(arg, nil)

args, err := qry.bindStructArgs(arg, nil)

zimnx · 2023-02-22T13:31:56Z

batchx.go

+// BindStruct binds query named parameters to values from arg using mapper. If
+// value cannot be found error is reported.


Suggested change

// BindStruct binds query named parameters to values from arg using mapper. If

// value cannot be found error is reported.

// BindStruct binds query named parameters to values from arg using a mapper.

// If value cannot be found an error is reported.

zimnx · 2023-02-22T13:32:25Z

batchx.go

+}
+
+// ExecuteBatch executes a batch operation and returns nil if successful
+// otherwise an error is returned describing the failure.


Suggested change

// otherwise an error is returned describing the failure.

// otherwise an error describing the failure.

zimnx · 2023-02-22T13:34:10Z

batchx_test.go

+// Running examples locally:
+// make run-scylla
+// make run-examples


remove it, it's not the right place to state it

zimnx · 2023-02-22T13:38:03Z

batchx_test.go

+// make run-scylla
+// make run-examples
+func TestBatch(t *testing.T) {
+	cluster := gocqlxtest.CreateCluster()


t.Parallel() if possible

zimnx · 2023-02-22T13:38:17Z

batchx_test.go

+	selectPlaylist := qb.Select("batch_test.playlists").Where(qb.Eq("id")).Query(session)
+
+	t.Run("batch inserts", func(t *testing.T) {
+		qrys := []batchQry{


t.Parallel() if possible

zimnx · 2023-02-22T13:39:38Z

batchx_test.go

+
+		b := session.NewBatch(gocql.LoggedBatch)
+		for _, qry := range qrys {
+			if r := b.BindStruct(qry.qry, qry.arg); r != nil {


Suggested change

if r := b.BindStruct(qry.qry, qry.arg); r != nil {

if err := b.BindStruct(qry.qry, qry.arg); err != nil {

applies in entire function

zimnx · 2023-02-22T13:42:26Z

batchx_test.go

+			t.Fatal("select song:", r)
+		}
+		if diff := cmp.Diff(gotSong, song); diff != "" {
+			t.Error("song mismatch", diff)


Suggested change

t.Error("song mismatch", diff)

t.Errorf("expected %v song, got %v, diff: %q", song, gotSong, diff)

zimnx · 2023-02-22T13:42:39Z

batchx_test.go

+			t.Fatal("select song:", r)
+		}
+		if diff := cmp.Diff(gotPlayList, playlist); diff != "" {
+			t.Error("playList mismatch", diff)


zimnx · 2023-02-22T13:43:55Z

Please squash commits into single one with descriptive commit message. Thanks

tehsphinx · 2023-03-03T15:32:59Z

@zimnx thx for the review. I applied the requested changes :)

tehsphinx · 2023-03-03T15:37:18Z

Note: adding t.Parallel() to the sub-test in batchx_test.go will close the session before the sub-test is done (defer session.Close() executes too early).

EDIT: adjusted to use t.Cleanup instead of defer.

zimnx

last two nits, thanks

zimnx · 2023-03-03T16:25:36Z

example_test.go

 	if err != nil {
 		t.Fatal("create table:", err)
 	}

 	playlistMetadata := table.Metadata{
-		Name:    "examples.playlists",
+		Name:    keyspace + ".playlists",


let's be consistent and use fmt.Sprintf here

right, missed those :)

zimnx · 2023-03-03T16:25:40Z

example_test.go

 		Columns: []string{"id", "title", "album", "artist", "song_id"},
 		PartKey: []string{"id"},
 		SortKey: []string{"title", "album", "artist", "song_id"},
 	}
 	playlistTable := table.New(playlistMetadata)

 	// Insert song using query builder.
-	insertSong := qb.Insert("examples.songs").
+	insertSong := qb.Insert(keyspace+".songs").


let's be consistent and use fmt.Sprintf here

tehsphinx · 2023-03-08T13:06:51Z

@zimnx Thanks! Adjusted 😄

zimnx

lgtm, thanks!

tehsphinx reviewed Jul 28, 2022

View reviewed changes

tehsphinx approved these changes Oct 7, 2022

View reviewed changes

mariosttass force-pushed the DT-5886 branch from 84dae7e to 6947635 Compare October 20, 2022 08:11

tehsphinx force-pushed the DT-5886 branch from 285f8fa to 1644121 Compare October 20, 2022 14:39

kaatinga reviewed Jan 11, 2023

View reviewed changes

batchx.go Show resolved Hide resolved

zimnx reviewed Feb 22, 2023

View reviewed changes

tehsphinx force-pushed the DT-5886 branch from f804d88 to cff2b05 Compare March 3, 2023 15:31

tehsphinx force-pushed the DT-5886 branch from cff2b05 to b722003 Compare March 3, 2023 15:35

tehsphinx force-pushed the DT-5886 branch from b722003 to bc41786 Compare March 3, 2023 15:38

zimnx reviewed Mar 3, 2023

View reviewed changes

add Batch wrapper with BindStruct method

071b93c

tehsphinx force-pushed the DT-5886 branch from bc41786 to 071b93c Compare March 8, 2023 13:06

zimnx approved these changes Mar 9, 2023

View reviewed changes

zimnx merged commit dec046b into scylladb:master Mar 9, 2023

tehsphinx deleted the DT-5886 branch March 9, 2023 13:44

		github.com/google/go-cmp v0.5.4
		github.com/maraino/go-mock v0.0.0-20180321183845-4c74c434cd3a

	func (s Session) NewBatch(typ gocql.BatchType) Batch {
	func (s Session) NewBatch(bt gocql.BatchType) Batch {

	arglist, err := qry.bindStructArgs(arg, nil)
	args, err := qry.bindStructArgs(arg, nil)

		// BindStruct binds query named parameters to values from arg using mapper. If
		// value cannot be found error is reported.

	// otherwise an error is returned describing the failure.
	// otherwise an error describing the failure.

	if r := b.BindStruct(qry.qry, qry.arg); r != nil {
	if err := b.BindStruct(qry.qry, qry.arg); err != nil {

	t.Error("song mismatch", diff)
	t.Errorf("expected %v song, got %v, diff: %q", song, gotSong, diff)

Adding batch wrapper on query builder #239

Adding batch wrapper on query builder #239

Conversation

mariosttass commented Jul 22, 2022

tehsphinx commented Jul 22, 2022 • edited Loading

tehsphinx commented Jul 28, 2022

tehsphinx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tehsphinx left a comment

Choose a reason for hiding this comment

tehsphinx commented Oct 19, 2022 • edited Loading

mmatczuk commented Oct 19, 2022

tehsphinx commented Oct 20, 2022 • edited Loading

tehsphinx commented Oct 20, 2022

mmatczuk commented Oct 21, 2022

tehsphinx commented Nov 14, 2022

tehsphinx commented Nov 30, 2022

tzach commented Nov 30, 2022

tehsphinx commented Jan 4, 2023 • edited Loading

kaatinga left a comment

Choose a reason for hiding this comment

tehsphinx commented Feb 21, 2023

mykaul commented Feb 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zimnx commented Feb 22, 2023

tehsphinx commented Mar 3, 2023

tehsphinx commented Mar 3, 2023 • edited Loading

zimnx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tehsphinx commented Mar 8, 2023

zimnx left a comment

Choose a reason for hiding this comment

tehsphinx commented Jul 22, 2022 •

edited

Loading

tehsphinx commented Oct 19, 2022 •

edited

Loading

tehsphinx commented Oct 20, 2022 •

edited

Loading

tehsphinx commented Jan 4, 2023 •

edited

Loading

tehsphinx commented Mar 3, 2023 •

edited

Loading