Skip to content
This repository was archived by the owner on Aug 23, 2023. It is now read-only.

update to work with cosmosdb #922

Merged
merged 3 commits into from
May 28, 2018
Merged

update to work with cosmosdb #922

merged 3 commits into from
May 28, 2018

Conversation

woodsaj
Copy link
Member

@woodsaj woodsaj commented May 23, 2018

add config settings for gocql's DisableInitialHostLookup setting. This is needed when using Cosmosdb as it does not report the internal topology of the service.

@woodsaj woodsaj requested a review from Dieterbe May 23, 2018 07:02
@woodsaj woodsaj force-pushed the cosmosdb branch 2 times, most recently from 988cd10 to baa8a5b Compare May 23, 2018 07:21
@@ -356,6 +358,8 @@ password = cassandra
create-keyspace = true
# File containing the needed schemas in case database needs initializing
schema-file = /etc/metrictank/schema-idx-cassandra.toml
# instruct the driver to not attempt to get host info from the system.peers table
disable-initial-host-lookup = false
Copy link
Contributor

Choose a reason for hiding this comment

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

i'm not a big fan of double negatives. can we call this initial-host-lookup = true ?

Copy link
Member Author

Choose a reason for hiding this comment

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

the config setting used by gocql is "DisableInitialHostLookup" so i think it makes sense to also use that.

@@ -170,7 +170,7 @@ func GetTTLTable(ttl uint32, windowFactor int, nameFormat string) ttlTable {
}
}

func NewCassandraStore(addrs, keyspace, consistency, CaPath, Username, Password, hostSelectionPolicy string, timeout, readers, writers, readqsize, writeqsize, retries, protoVer, windowFactor, omitReadTimeout int, ssl, auth, hostVerification bool, createKeyspace bool, schemaFile string, ttls []uint32) (*CassandraStore, error) {
func NewCassandraStore(addrs, keyspace, consistency, CaPath, Username, Password, hostSelectionPolicy string, timeout, readers, writers, readqsize, writeqsize, retries, protoVer, windowFactor, omitReadTimeout int, ssl, auth, hostVerification bool, createKeyspace bool, schemaFile string, ttls []uint32, disableInitialHostLookup bool) (*CassandraStore, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

we can make this more compact by bundling the bool params together

Copy link
Member Author

Choose a reason for hiding this comment

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

I will fix up this constructor in a separate PR.

var err error
tmpSession, err := cluster.CreateSession()
if err != nil {
log.Info("cassandra_store: session timeout: %s", cluster.Timeout.String())
log.Fatal(4, err.Error())
Copy link
Contributor

Choose a reason for hiding this comment

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

it's up to the caller to handle this error. (I checked and current callers already will exit when they get an error)

@@ -190,11 +190,15 @@ func NewCassandraStore(addrs, keyspace, consistency, CaPath, Username, Password,
}
cluster.Consistency = gocql.ParseConsistency(consistency)
cluster.Timeout = time.Duration(timeout) * time.Millisecond
cluster.ConnectTimeout = cluster.Timeout
Copy link
Contributor

Choose a reason for hiding this comment

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

this seems useful, but warrants a separate commit so we can mark it in the changelog

Copy link
Member Author

Choose a reason for hiding this comment

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

done

var err error
tmpSession, err := cluster.CreateSession()
if err != nil {
log.Info("cassandra_store: session timeout: %s", cluster.Timeout.String())
Copy link
Contributor

Choose a reason for hiding this comment

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

we generally don't log config params elsewhere, so i don't think we should do it for this value either

placeholders[i] = strconv.Itoa(int(p))
}
q := fmt.Sprintf("SELECT id, orgid, partition, name, interval, unit, mtype, tags, lastupdate from metric_idx where partition in (%s)", strings.Join(placeholders, ","))
iter := c.session.Query(q).Iter()
Copy link
Contributor

Choose a reason for hiding this comment

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

can you shed some more light on this? is this merely because cosmos doesn't understand the former syntax, or are there other reasons such as performance?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is just an optimization. I ran into issues on a dev cosmosdb instance due to how it limits requests. Basically, making the request with a single query rather then separate queries for each partition uses fewer resources.

@Dieterbe
Copy link
Contributor

can you tell me which other stack the docker-cosmos is based on?
according to the below it's very different to all the other stacks. and different in more ways than what's needed I think.

for i in docker-*; do echo $i; diff -N $i docker-cosmosdb | wc -l ; done                                                                                              ⏎
docker-chaos
486
docker-cluster
497
docker-cosmosdb
0
docker-dev
576
docker-dev-custom-cfg-kafka
312
docker-dev-scylla
534
docker-standard
532

@woodsaj
Copy link
Member Author

woodsaj commented May 25, 2018

can you tell me which other stack the docker-cosmos is based on?

it is not based on any of them. I built a stack that had everything I needed to test and verify things were working.

woodsaj added 2 commits May 25, 2018 20:17
Connecting to a cassandra cluster can sometimes take longer then the
default 600ms, especially on a dev cluster and when using TLS.
when using some managed Cassandra services, like Azure Cosmos DB, the
Cassandra services doesn't expose the internal cluster topology, so
we need to disable initial host lookup and just send requests to the
configured cluster address.
@Dieterbe Dieterbe merged commit 61fc5f9 into master May 28, 2018
@Dieterbe Dieterbe deleted the cosmosdb branch September 18, 2018 09:08
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants