Refactor Cassandra test utility for NoSQL support #4311

longquanzheng · 2021-07-10T03:49:20Z

What changed?

Create NoSQL interface for testing
Extract the logic of Cassandra to implement the interface
Remove unused code

Why?
For #3514
So that other NoSQL can run persistence test easily.

Once this is merged, we can start working on NoSQL support.
Though we still need two refactoring: 1) Refactoring schema tool to support NoSQL 2) Refactoring integ test to support NoSQL.

How did you test it?
existing tests.

Potential risks
Low risks. The refactoring is only for testing support for NoSQL

Release notes
No

Documentation Changes
No

coveralls · 2021-07-11T00:04:23Z

Pull Request Test Coverage Report for Build 40170d4a-e7d4-4658-b87f-510ada8cde5b

182 of 272 (66.91%) changed or added relevant lines in 24 files are covered.
53 unchanged lines in 9 files lost coverage.
Overall coverage increased (+0.04%) to 56.726%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
common/persistence/sql/sqlPersistenceTest.go	14	15	93.33%
tools/cassandra/handler.go	0	1	0.0%
common/persistence/nosql/nosqlplugin/cassandra/plugin.go	39	41	95.12%
common/persistence/nosql/nosqlplugin/types.go	3	5	60.0%
common/persistence/persistence-tests/persistenceTestBase.go	0	3	0.0%
common/persistence/nosql/nosqlplugin/cassandra/gocql/public/testBase.go	0	4	0.0%
tools/cli/adminDBScanCommand.go	0	6	0.0%
common/persistence/nosql/nosqlPersistenceTest.go	40	48	83.33%
common/persistence/nosql/nosqlplugin/cassandra/admin.go	36	53	67.92%
common/persistence/nosql/plugin.go	13	32	40.63%

Files with Coverage Reduction	New Missed Lines	%
common/task/weightedRoundRobinTaskScheduler.go	1	89.64%
common/task/fifoTaskScheduler.go	2	82.47%
tools/cli/adminDBScanCommand.go	2	0%
common/persistence/nosql/nosqlplugin/cassandra/workflow.go	3	50.23%
service/history/queue/timer_gate.go	3	95.83%
service/history/task/transfer_active_task_executor.go	4	69.73%
common/persistence/nosql/nosqlplugin/cassandra/workflowUtils.go	8	78.09%
common/persistence/nosql/nosqlplugin/cassandra/workflowParsingUtils.go	14	88.1%
common/persistence/sql/sqlExecutionStore.go	16	59.54%

Totals
Change from base Build 4362f1b4-15a1-4779-bb50-d505e78a289c:	0.04%
Covered Lines:	77586
Relevant Lines:	136773

💛 - Coveralls

yycptt

LGTM overall. I guess the change won't break our internal repo except for the naming change in TestBaseOptions struct? @Shaddoll

yycptt · 2021-07-20T04:46:02Z

common/persistence/nosql/store.go

+
+var supportedPlugins = map[string]nosqlplugin.Plugin{}
+
+// RegisterPlugin will register a SQL plugin


nit: NoSQL plugin. Three more in this file.

Done, 4 more actually :D thanks for the catch.

yycptt · 2021-07-20T04:48:27Z

common/persistence/nosql/store.go

@@ -0,0 +1,86 @@
+// Copyright (c) 2021 Uber Technologies, Inc.


nit: store.go sounds a bit confusing. Maybe plugin.go or registry.go?

Updated to plugin.go ,and also rename the one in sql/ for consistency.

yycptt · 2021-07-20T04:51:28Z

common/persistence/nosql/nosqlplugin/cassandra/gocql/client.go

-// NewClient gets a gocql client based registered object
-func NewClient() Client {
+// GetOrCreateClient gets a gocql client based registered object
+func GetOrCreateClient() Client {


nit: since it only returned registered client, maybe just GetClient()?

Yeah good point. I changed to GetRegisteredClient if you don't mind.

yycptt · 2021-07-20T04:58:10Z

tools/cassandra/handler.go

@@ -72,7 +71,7 @@ func verifyCompatibleVersion(
 		return nil
 	}

-	if ds.NoSQL.PluginName != cassandra_db.PluginName {


reusing the defined constant will be better than hardcoding I think? Anything I am missing here?

It's causing another cycle dependency. And since this file will be refactored soon to support NoSQL tools(after the refactor the cycle dep should be gone) so I don't want to spend time on fixing the dependency just for this constant here. I will leave a comment to explain why I don't use the constant.

yycptt · 2021-07-20T05:03:07Z

tools/cli/adminCommands.go

-		tmpl := "select domain from domains where id = ? "
-		query := session.Query(tmpl, domainID)
-		res, err := readOneRow(query)
+		domain, err := db.SelectDomain(ctx.Background(), &domainID, nil)


nit: ctx.Background() -> newContext(c)?

common/persistence/nosql/nosqlplugin/interfaces.go

yycptt · 2021-07-20T05:19:08Z

common/persistence/persistence-tests/testcluster/interfaces.go

+// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+// THE SOFTWARE.
+
+package testcluster


if it's for avoiding cycle dependency would you mind add some comments describing the problem?

longquanzheng changed the title ~~Refactor Cassandra test utility for NoSQL support~~ Refactor Cassandra test/tool utility for NoSQL support Jul 11, 2021

longquanzheng force-pushed the qlong-nosql-admin branch from 4163e94 to faf2c5f Compare July 12, 2021 01:19

longquanzheng changed the title ~~Refactor Cassandra test/tool utility for NoSQL support~~ Refactor Cassandra test utility for NoSQL support Jul 12, 2021

longquanzheng marked this pull request as ready for review July 12, 2021 01:24

longquanzheng mentioned this pull request Jul 12, 2021

Proposal: support plugins for NoSQL database #3514

Closed

longquanzheng requested review from yycptt, yux0 and Shaddoll July 12, 2021 16:47

longquanzheng force-pushed the qlong-nosql-admin branch from 69fad9b to 1f5fa38 Compare July 15, 2021 22:12

github-actions bot force-pushed the qlong-nosql-admin branch 2 times, most recently from dd166d6 to 5b3e1ed Compare July 19, 2021 22:20

yycptt approved these changes Jul 20, 2021

View reviewed changes

longquanzheng added 17 commits July 20, 2021 22:30

private

b113809

private

c33d043

simplify gocql Client usage

9f15471

rename registry method

f0ddc7a

types.go

652b039

plugin interface and comment

ed99822

empty spaces

9a33eaa

nosql store

a304370

cassandra plugin

a8465c5

plumb DB

881a6ae

adminDB queryOneRow

8906e91

queryOneRow

345dded

switch to dynamic loading

a80a2d2

fix

4c08d21

fix test

097d90f

testCluster

2f8c5fc

fix cycle dependency

53c0d93

longquanzheng added 16 commits July 20, 2021 22:30

Setup and Teardown for Cassandra

00607d6

fix cycle dependency

fc77e25

remove unused code

66b5736

use admindb

11e2bb7

fix test

a2eb61a

plugin

9e2cf73

move to nosql

e178dd3

rename

adb144d

remove unused

65172b9

fix test

ce092a3

fix

ea15941

remove queryOneRow

97982cd

address comments

2cc71fa

gocql.GetRegisteredClient

58bc609

use newContext

b589f4b

comments

6af33ce

longquanzheng force-pushed the qlong-nosql-admin branch from 5b3e1ed to 6af33ce Compare July 21, 2021 05:41

fmt

4d3a1dc

longquanzheng added the automerge label Jul 21, 2021

github-actions bot merged commit 66f2f26 into master Jul 21, 2021

github-actions bot deleted the qlong-nosql-admin branch July 21, 2021 06:02

github-actions bot removed the automerge label Jul 21, 2021

longquanzheng mentioned this pull request Jul 26, 2021

Fix CLI admin domain bug that didn't load Cassandra plugin #4330

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor Cassandra test utility for NoSQL support #4311

Refactor Cassandra test utility for NoSQL support #4311

longquanzheng commented Jul 10, 2021 •

edited

Loading

coveralls commented Jul 11, 2021 •

edited

Loading

yycptt left a comment

yycptt Jul 20, 2021

longquanzheng Jul 21, 2021

yycptt Jul 20, 2021

longquanzheng Jul 21, 2021

yycptt Jul 20, 2021

longquanzheng Jul 21, 2021

yycptt Jul 20, 2021

longquanzheng Jul 21, 2021 •

edited

Loading

yycptt Jul 20, 2021

yycptt Jul 20, 2021

longquanzheng Jul 21, 2021


		var supportedPlugins = map[string]nosqlplugin.Plugin{}

		// RegisterPlugin will register a SQL plugin

		@@ -0,0 +1,86 @@
		// Copyright (c) 2021 Uber Technologies, Inc.

Refactor Cassandra test utility for NoSQL support #4311

Refactor Cassandra test utility for NoSQL support #4311

Conversation

longquanzheng commented Jul 10, 2021 • edited Loading

coveralls commented Jul 11, 2021 • edited Loading

Pull Request Test Coverage Report for Build 40170d4a-e7d4-4658-b87f-510ada8cde5b

💛 - Coveralls

yycptt 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

longquanzheng Jul 21, 2021 • edited Loading

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

longquanzheng commented Jul 10, 2021 •

edited

Loading

coveralls commented Jul 11, 2021 •

edited

Loading

longquanzheng Jul 21, 2021 •

edited

Loading