Don't handle database placholders when specifying connection_string parameter

[cosmo0920]

Fixes #116.

I didn't notice this functionailty breakage.... If @database is not set and used @connection_string, we shouldn't try to extract @database placeholder. It is a pitfall.

Signed-off-by: Hiroshi Hatake hatake@clear-code.com

[tomohiro-n]

Thanks for the quick patch! I think this would fix the issue. To keep what the previous commit https://github.com/fluent/fluent-plugin-mongo/commit/803821a8e50b2b10d4a684c7e5d697ead6214de6 wants to achieve, maybe setting @database based on the @connection_string would be an ideal fix but I'm not so sure.

[cosmo0920]

To keep what the previous commit 803821a wants to achieve, maybe setting @database based on the @connection_string would be an ideal fix but I'm not so sure.

Really? The previous commit wants to support dynamic database functionality for @database parameter, but it is not for @connection_string.

[tomohiro-n]

@connection_string might consist of the database as I put on the issue #116. In the example of, mongodb://testUser:testPass@localhost:27017/testDb, I thought it might be better to extract the testDb from this string or from an instance of the connection. Having said that, I don't think I fully understand your intention and how this works yet. I just read some codes for my quick workaround only. Please go ahead of the either approach :)

[cosmo0920]

In the example of, mongodb://testUser:testPass@localhost:27017/testDb, I thought it might be better to extract the testDb from this string or from an instance of the connection.

This is useless.

Because two reasons:

@connection_string is used for connection_string mode: https://github.com/fluent/fluent-plugin-mongo/blob/master/lib/fluent/plugin/out_mongo.rb#L183-L184 In connection_string mode, mongo ruby driver doesn't use @database for connecting.

@database is used for normal mode: https://github.com/fluent/fluent-plugin-mongo/blob/master/lib/fluent/plugin/out_mongo.rb#L186-L189 In normal mode, mongo ruby driver which refers @database and @port for connecting:

Why should we extract database information from connection_string even if not referred? Just for symmetry of implementation?

[tomohiro-n]

Thanks for the detailed explanation, it explains this PR is good to merge for me.

It's a bit off the topic but by the word normal, I'm in an impression of the normal way is highly recommended? I chose to use connection_string because I was not certain how the normal way handles mongodb+srv protocol which may use cluster's hostname rather than the set of underlying database servers. I looked at the closed issue #100 and chose to simply use connection_string. I would like to switch to the normal way if the way handles cluster's hostname correctly at the host property because it makes easier to control what is set to ConfigMap and to Secret in my k8s settings. For example,

connection_string mongodb+srv://userName:passWord@clusterone.mongodb.abc.com:27017/testDb

and

database testDb
host clusterone.mongodb.abc.com
port 27017
user userName
password passWord

are totally equivalent in the fluent.conf ?

Your feedback would be appreciated! Thank you!

[cosmo0920]

@tomohiro-n Not equivalent. See:

https://github.com/mongodb/mongo-ruby-driver/blob/a554f944521d90a7fbd90298623938693425e5f0/lib/mongo/client.rb#L323-L329 URI.get implementation is: https://github.com/mongodb/mongo-ruby-driver/blob/a554f944521d90a7fbd90298623938693425e5f0/lib/mongo/uri.rb#L212-L230

[tomohiro-n]

@cosmo0920

Thank you. So I think we have two ways to connect to the cluster with this plugin.

1. use connection_string with @type mongo
2. use nodes and replica_set with @type mongo_replset

Is this correct? and if yes, which way is preferred? I think Mongo recommends to use mongodb+srv with recent drivers(2.5+ for Ruby one) but I suspect the 2nd approach might be still preferred in this plugin?

[cosmo0920]

@tomohiro-n It depends on your Web application requirements, using MongoDB server (or cluster?) versions, and using network (including DNS). I'm not sure which is better for your situation.

[tomohiro-n]

@cosmo0920 Got it. Thanks so much!

[cosmo0920]

@tomohiro-n If you still have more questions, please feel free to send Fluentd Community Slack or Mailing List.

[repeatedly]

Patch itself seems good. Merged.