Increase number of connectivity options #31

squall0gd · 2017-04-06T11:13:58Z

Fixes #

Summary of changes:

Following configuration flags has been added:
- port - described here
- timeout - described here
- connectionTimeout - described here
- initialHostLookup - described here
- ignorePeerAddr- described here
- keyspace - described here
- createKeyspace - decide that keyspace should be created or not.
Don't force Keyspace name to snap

How to verify it:

Build and run with external Cassandra instance.

Testing done:

Tested with production Cassandra

TBD:

~~Unit tests~~
~~Integration tests~~

A picture of a snapping turtle (not required but encouraged):

Before change:
If your Cassandra cluster is placed in private network with public endpoint and your administrator give you so little permissions(so you cannot even create your own namespace), you cannot use this plugin.

After change:
This PR brings additional options to configure connection to Cassandra cluster. Following options has been added:

Flexible keyspace and table names
Ability to decide that keyspace should be created or not(if your admin doesn't bring you permissions to create your own keyspace and you try to create it, your snap task will fail. Now, you can simply skip keyspace creation)
Ignore discovering nodes in cluster(if your cluster nodes have got private addresses and public keyspace, than you won't connect to particular nodes in cluster with their private addresses)
Flexible Timeout and first connection Timeout(in case of big latency on your connection, you will need to increase this two parameters)

squall0gd · 2017-04-06T11:31:50Z

Resolves #30

squall0gd · 2017-04-06T11:33:57Z

@candysmurf what is the reason of using your cassandra image instead of official one?

candysmurf · 2017-04-07T00:55:39Z

cassandra/cassandra.go

@@ -75,6 +83,41 @@ func (cas *CassandraPublisher) GetConfigPolicy() (*cpolicy.ConfigPolicy, error)
 	serverAddrRule.Description = "Cassandra server"
 	config.Add(serverAddrRule)

+	portRule, err := cpolicy.NewIntegerRule(portRuleKey, false, 9042)
+	handleErr(err)
+	serverAddrRule.Description = "Cassandra server port, default: 9042"


portRule.Description

candysmurf · 2017-04-07T00:55:55Z

cassandra/cassandra.go

+
+	timeoutRule, err := cpolicy.NewIntegerRule(timeoutRuleKey, false, 2)
+	handleErr(err)
+	serverAddrRule.Description = "Connection timeout in seconds, default: 2"


timeoutRule.Description

candysmurf · 2017-04-07T00:56:11Z

cassandra/cassandra.go

+
+	connectionTimeoutRule, err := cpolicy.NewIntegerRule(connectionTimeoutRuleKey, false, 2)
+	handleErr(err)
+	serverAddrRule.Description = "Initial connection timeout in seconds, default: 2"


connectionTimeoutRule

candysmurf · 2017-04-07T00:56:35Z

cassandra/cassandra.go

+
+	initialHostLookupRule, err := cpolicy.NewBoolRule(initialHostLookupRuleKey, false, true)
+	handleErr(err)
+	serverAddrRule.Description = "Lookup for cluster hosts information, default: true"


initialHostLookupRule

candysmurf · 2017-04-07T00:57:04Z

cassandra/cassandra.go

+
+	ignorePeerAddrRule, err := cpolicy.NewBoolRule(ignorePeerAddrRuleKey, false, false)
+	handleErr(err)
+	serverAddrRule.Description = "Turn off cluster hosts tracking, default: false"


ignorePeerAddrRule

candysmurf · 2017-04-07T01:02:54Z

cassandra/cassandra.go

+	serverAddrRule.Description = "Turn off cluster hosts tracking, default: false"
+	config.Add(ignorePeerAddrRule)
+
+	dontCreateKeyspaceRule, err := cpolicy.NewBoolRule(createKeyspaceRuleKey, false, true)


createKeyspaceRule

candysmurf · 2017-04-07T01:06:54Z

cassandra/cassandra.go

@@ -223,7 +288,7 @@ func getSslOptions(cfg map[string]ctypes.ConfigValue) *sslOptions {

 func handleErr(e error) {
 	if e != nil {
-		log.Fatal(e.Error())
+		log.Fatalf("ABC %s", e.Error())


what does "ABC" represent here?

That was for debug purposes, I've forgot to remove it :/

candysmurf · 2017-04-07T01:14:42Z

cassandra/client.go

 		log.Fatal(err.Error())
 	}

-	if err := session.Query(createTagTableCQL).Exec(); err != nil {
+	if err := session.Query(fmt.Sprintf(createTagTableCQL, co.keyspace)).Exec(); err != nil {


should we allow a user to specify the table name similar like the keyspace but default to snap?

we can do this, but I propose to use metrics instead of snap ;)

What about tag table?

@squall0gd, tag table is a by-product. Let us leave it. Also metric as the table name seems fine if we don't want to change too much now.

candysmurf · 2017-04-07T01:20:10Z

cassandra/client.go

-	cluster := createCluster(co.server)
+func createKeyspace(co clientOptions) {
+	cluster := createCluster(co)
+	session, err := cluster.CreateSession()


using getInstance(), singleton session is desired.

I know, but you need to create keyspace separately. Your configured session isn't able to switch to new keyspace. This case will remove keyspace name from rest of queries. Another approach is to format each query with keyspace name. What do you think?

@squall0gd, yes, keep the keyspace in the query.

candysmurf · 2017-04-07T01:20:22Z

cassandra/client.go

+	if err != nil {
+		log.Fatal(err.Error())
+	}
+	defer session.Close()


don't close out the session here. Leaving it open

candysmurf · 2017-04-07T01:22:14Z

@squall0gd, using my image was for the medium test to check if the Cassandra is up. It's not required.

candysmurf · 2017-04-07T01:29:17Z

cassandra/cassandra.go

+	connectionTimeoutRuleKey   = "connectionTimeout"
+	initialHostLookupRuleKey   = "initialHostLookup"
+	ignorePeerAddrRuleKey      = "ignoreRuleKey"
+	createKeyspaceRuleKey      = "createKeyspace"


it seems that we don't need this property. The keyspace is default to snap. Keyspace name suffices.

My case is that cassandra administrator give me access to cassandra instance with prepared keyspace for me. I cannot create any other keyspace, so even attempt is killing publisher flow.

marcintao · 2017-05-04T11:54:03Z

cassandra/cassandra.go


-	passwordRule, err := cpolicy.NewStringRule(passwordRuleKey, false, "")
+	dontCreateKeyspaceRule, err := cpolicy.NewBoolRule(createKeyspaceRuleKey, false, true)


Remove negation from option name (dont-), as it's value is used directly further in the code.

good catch, I've forgot about this one :)

marcintao

LGTM

PatrykMatyjasek · 2017-05-11T09:13:00Z

LGTM

candysmurf

@squall0gd, please address my feedback. Your dynamic table name implementation simply does not work now.

candysmurf · 2017-05-15T00:19:12Z

cassandra/client.go

 	session, err := cluster.CreateSession()
 	if err != nil {
 		log.Fatal(err.Error())
 	}

-	if err := session.Query(createKeyspaceCQL).Exec(); err != nil {
-		log.Fatal(err.Error())
+	if co.createKeyspace {


Please remove this createKeyspace flag. It's redundant. As the CQL itself already does this.

No I cannot - it's required :/ if you try to create keyspace and you don't have permissions to your cassandra cluster then your snap task will fail b/c cassandra publisher will panic b/c gocql will exit with panic ;). This flag is simply to handle situation when you know that you don't have permissions.

If you've got permissions to create keyspace, then you're right, but you cannot simply predict if remote cluster give you required permissions or not

@squall0gd, user should have privilege set inside Cassandra. we shouldn't use this flag.

@candysmurf you cannot expect that your client will have this permissions. And as far as we cannot do that, we need this flag to run tasks for enterprise ready cluster.

Here you've got some proof of importance of this flag.

File task_w_creating_keyspace.yml - emulates situation, when this flag isn't needed
File task_wo_creating_keyspace.yml - prevents from creating keyspace.

As you can see creating keyspace is killing first task, while second task is up and running.

candysmurf · 2017-05-15T00:25:16Z

cassandra/client.go

-	createTableCQL    = "CREATE TABLE IF NOT EXISTS snap.metrics (ns  text, ver int, host text, time timestamp, valType text, doubleVal double, strVal text, boolVal boolean, tags map<text,text>, PRIMARY KEY ((ns, ver, host), time),) WITH CLUSTERING ORDER BY (time DESC);"
-	createTagTableCQL = "CREATE TABLE IF NOT EXISTS snap.tags (key  text, val text, time timestamp, ns text, ver int, host text, valType text, doubleVal double, strVal text, boolVal boolean, tags map<text,text>, PRIMARY KEY ((key, val), time),) WITH CLUSTERING ORDER BY (time DESC);"
+	createKeyspaceCQL = "CREATE KEYSPACE IF NOT EXISTS %s WITH REPLICATION = {'class': 'SimpleStrategy', 'replication_factor': 1};"
+	createTableCQL    = "CREATE TABLE IF NOT EXISTS %s.metrics (ns  text, ver int, host text, time timestamp, valType text, doubleVal double, strVal text, boolVal boolean, tags map<text,text>, PRIMARY KEY ((ns, ver, host), time),) WITH CLUSTERING ORDER BY (time DESC);"


the table name is always metrics although the intent is to have user-specified table name but default to metrics.

Great catch! Thanks :)

candysmurf · 2017-05-24T19:39:47Z

LGTM, @squall0gd, we can merge this PR after you squash all the commits. thanks.

squall0gd requested a review from akwasnie April 6, 2017 11:13

squall0gd self-assigned this Apr 6, 2017

squall0gd added the help wanted label Apr 6, 2017

squall0gd requested review from candysmurf and katarzyna-z April 6, 2017 11:32

candysmurf suggested changes Apr 7, 2017

View reviewed changes

squall0gd mentioned this pull request Apr 7, 2017

Add additional configuration options intelsdi-x/swan#588

Merged

3 tasks

marcintao reviewed May 4, 2017

View reviewed changes

marcintao approved these changes May 11, 2017

View reviewed changes

candysmurf reviewed May 15, 2017

View reviewed changes

candysmurf approved these changes May 24, 2017

View reviewed changes

Increase number of connectivity options

8c29070

squall0gd force-pushed the squall0gd/new-connection-options branch from c62ff50 to 8c29070 Compare May 25, 2017 11:10

squall0gd merged commit b979cb3 into master May 25, 2017

squall0gd deleted the squall0gd/new-connection-options branch May 25, 2017 11:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Increase number of connectivity options #31

Increase number of connectivity options #31

squall0gd commented Apr 6, 2017 •

edited

Loading

squall0gd commented Apr 6, 2017

squall0gd commented Apr 6, 2017

candysmurf Apr 7, 2017

candysmurf Apr 7, 2017

candysmurf Apr 7, 2017

candysmurf Apr 7, 2017

candysmurf Apr 7, 2017

candysmurf Apr 7, 2017

candysmurf Apr 7, 2017

squall0gd Apr 7, 2017

candysmurf Apr 7, 2017

squall0gd Apr 7, 2017

squall0gd Apr 7, 2017

candysmurf Apr 18, 2017

candysmurf Apr 7, 2017

squall0gd Apr 7, 2017

candysmurf Apr 18, 2017

candysmurf Apr 7, 2017

candysmurf commented Apr 7, 2017 •

edited

Loading

candysmurf Apr 7, 2017

squall0gd Apr 7, 2017

marcintao May 4, 2017

squall0gd May 4, 2017

marcintao left a comment

PatrykMatyjasek commented May 11, 2017

candysmurf left a comment

candysmurf May 15, 2017

squall0gd May 15, 2017 •

edited

Loading

candysmurf May 18, 2017

squall0gd May 22, 2017

squall0gd May 22, 2017

candysmurf May 15, 2017

squall0gd May 15, 2017

candysmurf commented May 24, 2017


		passwordRule, err := cpolicy.NewStringRule(passwordRuleKey, false, "")
		dontCreateKeyspaceRule, err := cpolicy.NewBoolRule(createKeyspaceRuleKey, false, true)

Increase number of connectivity options #31

Increase number of connectivity options #31

Conversation

squall0gd commented Apr 6, 2017 • edited Loading

squall0gd commented Apr 6, 2017

squall0gd commented Apr 6, 2017

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

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

candysmurf commented Apr 7, 2017 • 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

marcintao left a comment

Choose a reason for hiding this comment

PatrykMatyjasek commented May 11, 2017

candysmurf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

squall0gd May 15, 2017 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

candysmurf commented May 24, 2017

squall0gd commented Apr 6, 2017 •

edited

Loading

candysmurf commented Apr 7, 2017 •

edited

Loading

squall0gd May 15, 2017 •

edited

Loading