Add support for external db for schema management in mongodb connector #8956

academy-codex · 2021-08-25T06:25:48Z

Aim: Fixes #8887
Description:

Added a configuration property to capture the name of the mongo database to be used by trino for schema management for the different other databases and collections on the mongodb cluster.
The property is optional and in the case it is not provided, the default implementation of _schema collection will happen.
The trino mongodb connector will create collections inside the configured db as trino_<<db_name>>_schema format and the collections will have documents of schemas of the collections in the <<db_name>>.
Introduced a flag to enable/disable creation of indexes. The user should have the feature to make it optional in case the user is okay with the penalty of query execution time without indexing.

academy-codex · 2021-08-25T06:27:48Z

@hashhar Kindly have a look over the draft changes done.

hashhar

Some initial comments.

plugin/trino-mongodb/src/main/java/io/trino/plugin/mongodb/MongoSession.java

plugin/trino-mongodb/src/main/java/io/trino/plugin/mongodb/MongoClientConfig.java

hashhar · 2021-08-26T13:46:24Z

Re: #8956 (comment)

I see this is for the collection names. Earlier it was just _schema because every database had one such collection.
Now a single database may have multiple collections.

We need to preserve backcompat (which is possible by not setting the db-name).
And we need to avoid collision of collection name.

I think the cleanest solution is to make collection-name and database-name configs mutually exclusive.
If db-name is specified each collection in the db is named the same as the collection whose schema they represent. That way it's really easy for users/admins to find the schema for a given collection.

If collection-name is specified then the code behaves as the old impl where each DB has one collection with the name specified in collection-name.

academy-codex · 2021-08-26T14:05:48Z

@hashhar I see your point. Let me make the changes and commit.

hashhar · 2021-08-26T14:22:07Z

One more question : different DBs can have collections with the same name. How does it work with the new mechanism you are adding?

academy-codex · 2021-08-26T14:27:21Z

One more question : different DBs can have collections with the same name. How does it work with the new mechanism you are adding?

So this should be fine actually. For example, you have have database X and database Y and a meta data database META. Inside meta 2 collections will be created with the same name as database names. META will have 2 collections X and Y.

Lets say X and Y both have a same table name sample. So inside META we only query in X collection where tableName="sample" for sample table of X and for Y we query in Y collection.

Both tables with same name but have entries in different collections in meta data database because they are from different databases.

Hope i was able to explain ?

academy-codex · 2021-08-26T14:48:15Z

@hashhar Did some changes. Please have a look once you get time :)
Thanks.

sumannewton · 2021-08-26T15:31:41Z

docs/src/main/sphinx/connector/mongodb.rst

@@ -59,6 +59,7 @@ Property Name                              Description
 ``mongodb.write-concern``                  The write concern
 ``mongodb.required-replica-set``           The required replica set name
 ``mongodb.cursor-batch-size``              The number of elements to return in a batch
+``mongodb.schema-database``                The database to use for schema management


This property should actually be placed right after mongodb.schema-collection.

Since we are improving the metadata management, there should be a point in both that only either of these will be used. Not both.

mongodb.schema-collection should be marked as deprecated. And we better remove it after couple of releases.

mongodb.schema-collection should be marked as deprecated. And we better remove it after couple of releases.

I disagree. I don't think we would like to break the backward compatibility.

mongodb.schema-collection should be marked as deprecated. And we better remove it after couple of releases.

I disagree. I don't think we would like to break the backward compatibility.

Alright. Makes sense.

The below comment isn't addressed. I think right "before" is better considering the hierarchy.

This property should actually be placed right after mongodb.schema-collection.

plugin/trino-mongodb/src/main/java/io/trino/plugin/mongodb/MongoSession.java

ebyhr

Please fix TestMongoClientConfig and add some tests.

plugin/trino-mongodb/src/main/java/io/trino/plugin/mongodb/MongoSession.java

plugin/trino-mongodb/src/main/java/io/trino/plugin/mongodb/MongoClientConfig.java

plugin/trino-mongodb/src/main/java/io/trino/plugin/mongodb/MongoSession.java

hashhar

Some more comments.

The impl looks good now. Can you also add some add tests too to make this is working as intended?

docs/src/main/sphinx/connector/mongodb.rst

plugin/trino-mongodb/src/main/java/io/trino/plugin/mongodb/MongoSession.java

academy-codex · 2021-08-27T19:59:30Z

Please fix TestMongoClientConfig and add some tests.

@ebyhr @hashhar Please check if all looks good now. I shall add the test cases and commit thereafter.

academy-codex · 2021-08-28T20:42:21Z

Added the fixes for the test cases

ebyhr

Please separate into two commits, index-option and external-db.

plugin/trino-mongodb/src/main/java/io/trino/plugin/mongodb/MongoClientConfig.java

ebyhr · 2021-08-29T02:31:49Z

docs/src/main/sphinx/connector/mongodb.rst

@@ -59,6 +59,7 @@ Property Name                              Description
 ``mongodb.write-concern``                  The write concern
 ``mongodb.required-replica-set``           The required replica set name
 ``mongodb.cursor-batch-size``              The number of elements to return in a batch
+``mongodb.schema-database``                The database to use for schema management


The below comment isn't addressed. I think right "before" is better considering the hierarchy.

This property should actually be placed right after mongodb.schema-collection.

plugin/trino-mongodb/src/main/java/io/trino/plugin/mongodb/MongoClientConfig.java

ebyhr

Almost good to me. Please squash and separate into 2 commits (create-index and schema-database) when applying comments.

docs/src/main/sphinx/connector/mongodb.rst

plugin/trino-mongodb/src/test/java/io/trino/plugin/mongodb/TestMongoClientConfig.java

plugin/trino-mongodb/src/main/java/io/trino/plugin/mongodb/MongoClientConfig.java

docs/src/main/sphinx/connector/mongodb.rst

academy-codex · 2021-11-20T12:40:53Z

@ebyhr @hashhar Addressed all review comments. Let me know if it's good to merge :)

ebyhr

Let's add TestMongoExternalDatabaseConnectorSmokeTest that extends BaseMongoConnectorSmokeTest with mongodb.schema-database config property.

ebyhr · 2021-11-22T01:58:17Z

docs/src/main/sphinx/connector/mongodb.rst

 ``mongodb.schema-collection``              A collection which contains schema information
+``mongodb.create-index-for-schema.enabled``Create an index for schema collection when it doesn't exist


Please check build failure.

ebyhr · 2021-11-22T01:59:39Z

plugin/trino-mongodb/src/main/java/io/trino/plugin/mongodb/MongoSession.java

@@ -91,7 +91,7 @@

 public class MongoSession
 {
-    private static final Logger log = Logger.get(MongoSession.class);
+    private static final Logger log = Logger.get(io.trino.plugin.mongodb.MongoSession.class);


Revert unrelated change.

ebyhr · 2021-11-22T02:01:10Z

plugin/trino-mongodb/src/main/java/io/trino/plugin/mongodb/MongoSession.java

@@ -147,6 +151,7 @@ public void shutdown()
    public List<String> getAllSchemas()
    {
        return ImmutableList.copyOf(client.listDatabaseNames()).stream()
+                .filter(name -> schemaDatabase.map(s -> !name.equals(s)).orElse(true))


Rename s to schema.

ebyhr · 2021-11-22T02:05:52Z

plugin/trino-mongodb/src/test/java/io/trino/plugin/mongodb/TestMongoClientConfig.java

@@ -54,6 +56,7 @@ public void testExplicitPropertyMappings()
    {
        Map<String, String> properties = new ImmutableMap.Builder<String, String>()
                .put("mongodb.schema-collection", "_my_schema")
+                .put("mongodb.enable-schema-create-index", "true")


Please correct the property name.

ebyhr · 2021-11-22T02:06:13Z

plugin/trino-mongodb/src/test/java/io/trino/plugin/mongodb/TestMongoClientConfig.java

+    {
+        Map<String, String> properties = new ImmutableMap.Builder<String, String>()
+                .put("mongodb.schema-database", "trino_meta_data_db")
+                .put("mongodb.enable-schema-create-index", "true")


Same as the above comment.

mosabua · 2022-10-28T01:59:12Z

👋 @academy-codex - this PR has become inactive. If you're still interested in working on it, please let us know.

We're working on closing out old and inactive PRs, so if you're too busy or this has too many merge conflicts to be worth picking back up, we'll be making another pass to close it out in a few weeks.

colebow · 2022-11-30T22:28:59Z

Closing this one out due to inactivity, but please reopen if you would like to pick this back up.

academy-codex · 2023-05-29T08:13:56Z

Hey! I want to pick this back up :)

mosabua · 2023-05-29T17:02:44Z

You can reopen the PR .. and then we can continue

cla-bot bot added the cla-signed label Aug 25, 2021

academy-codex requested a review from hashhar August 25, 2021 06:26

ebyhr self-requested a review August 25, 2021 14:46

hashhar reviewed Aug 26, 2021

View reviewed changes

sumannewton reviewed Aug 26, 2021

View reviewed changes

plugin/trino-mongodb/src/main/java/io/trino/plugin/mongodb/MongoSession.java Outdated Show resolved Hide resolved

ebyhr reviewed Aug 26, 2021

View reviewed changes

hashhar reviewed Aug 26, 2021

View reviewed changes

academy-codex requested a review from hashhar August 28, 2021 20:42

academy-codex mentioned this pull request Aug 28, 2021

Add support for MongoDB Atlas Data Lake #4544

Open

academy-codex requested a review from ebyhr August 29, 2021 07:33

ebyhr reviewed Sep 5, 2021

View reviewed changes

ebyhr self-requested a review September 29, 2021 05:23

ebyhr reviewed Oct 11, 2021

View reviewed changes

ebyhr mentioned this pull request Oct 26, 2021

Support connection string URI in MongoDB connector #9770

Closed

academy-codex force-pushed the mr_mongodb_ext_schema_mgmt branch from d659486 to f2f8beb Compare November 20, 2021 12:31

academy-codex requested a review from ebyhr November 20, 2021 12:40

ebyhr reviewed Nov 22, 2021

View reviewed changes

ebyhr assigned academy-codex Apr 18, 2022

colebow closed this Nov 30, 2022

ebyhr mentioned this pull request Nov 30, 2022

MongoDB connector to have configurable database for _schema collection #8887

Open

academy-codex reopened this May 29, 2023

github-actions bot added docs mongodb MongoDB connector labels May 29, 2023

academy-codex closed this May 29, 2023

academy-codex force-pushed the mr_mongodb_ext_schema_mgmt branch from 2e051dc to 6a4e483 Compare May 29, 2023 09:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for external db for schema management in mongodb connector #8956

Add support for external db for schema management in mongodb connector #8956

academy-codex commented Aug 25, 2021

academy-codex commented Aug 25, 2021

hashhar left a comment

hashhar commented Aug 26, 2021

academy-codex commented Aug 26, 2021

hashhar commented Aug 26, 2021

academy-codex commented Aug 26, 2021 •

edited

Loading

academy-codex commented Aug 26, 2021

sumannewton Aug 26, 2021

ebyhr Aug 26, 2021

sumannewton Aug 26, 2021

ebyhr Aug 29, 2021

academy-codex Sep 6, 2021

ebyhr left a comment

hashhar left a comment

academy-codex commented Aug 27, 2021

academy-codex commented Aug 28, 2021

ebyhr left a comment

ebyhr Aug 29, 2021

ebyhr left a comment

academy-codex commented Nov 20, 2021

ebyhr left a comment

ebyhr Nov 22, 2021

ebyhr Nov 22, 2021

ebyhr Nov 22, 2021

ebyhr Nov 22, 2021

ebyhr Nov 22, 2021

mosabua commented Oct 28, 2022

colebow commented Nov 30, 2022

academy-codex commented May 29, 2023

mosabua commented May 29, 2023

		``mongodb.schema-collection`` A collection which contains schema information
		``mongodb.create-index-for-schema.enabled``Create an index for schema collection when it doesn't exist

Add support for external db for schema management in mongodb connector #8956

Add support for external db for schema management in mongodb connector #8956

Conversation

academy-codex commented Aug 25, 2021

academy-codex commented Aug 25, 2021

hashhar left a comment

Choose a reason for hiding this comment

hashhar commented Aug 26, 2021

academy-codex commented Aug 26, 2021

hashhar commented Aug 26, 2021

academy-codex commented Aug 26, 2021 • edited Loading

academy-codex commented Aug 26, 2021

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

ebyhr left a comment

Choose a reason for hiding this comment

hashhar left a comment

Choose a reason for hiding this comment

academy-codex commented Aug 27, 2021

academy-codex commented Aug 28, 2021

ebyhr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebyhr left a comment

Choose a reason for hiding this comment

academy-codex commented Nov 20, 2021

ebyhr 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

mosabua commented Oct 28, 2022

colebow commented Nov 30, 2022

academy-codex commented May 29, 2023

mosabua commented May 29, 2023

academy-codex commented Aug 26, 2021 •

edited

Loading