Skip to content

Introduce MongoTransactionManager#19990

Merged
Praveen2112 merged 3 commits intotrinodb:masterfrom
Praveen2112:praveen/mongodb_cleanup
Dec 11, 2023
Merged

Introduce MongoTransactionManager#19990
Praveen2112 merged 3 commits intotrinodb:masterfrom
Praveen2112:praveen/mongodb_cleanup

Conversation

@Praveen2112
Copy link
Copy Markdown
Member

Description

Minor cleanups in Mongo connector which introduces MongoTransactionManager

Additional context and related issues

Release notes

(x) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
( ) Release notes are required, with the following suggested text:

# Section
* Fix some things. ({issue}`issuenumber`)

@cla-bot cla-bot bot added the cla-signed label Dec 1, 2023
@github-actions github-actions bot added the mongodb MongoDB connector label Dec 1, 2023
@Praveen2112 Praveen2112 force-pushed the praveen/mongodb_cleanup branch from ba33384 to e0c7c03 Compare December 1, 2023 12:59
Copy link
Copy Markdown
Contributor

@krvikash krvikash left a comment

Choose a reason for hiding this comment

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

LGTM. Some comments.

if (tableHandle == null) {
throw new TableNotFoundException(schemaTableName, format("Table '%s.%s' not found", database, collection), null);
}
RemoteTableName remoteTableName = metadata.getTableHandle(session, new SchemaTableName(database, collection)).getRemoteTableName();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why not use tableHandle.getRemoteTableName(); here? We can avoid calling getTableHandle again?

parseFilter(filter);

MongoTableHandle tableHandle = new MongoTableHandle(new SchemaTableName(database, collection), remoteTableName, Optional.of(filter));
tableHandle = new MongoTableHandle(new SchemaTableName(database, collection), remoteTableName, Optional.of(filter));
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

can we use schemaTableName here instead new SchemaTableName(database, collection)?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

readOnly and autoCommit is unused

Copy link
Copy Markdown
Contributor

@marcinsbd marcinsbd Dec 5, 2023

Choose a reason for hiding this comment

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

nit: I would split this method from this line

@Praveen2112 Praveen2112 force-pushed the praveen/mongodb_cleanup branch from e0c7c03 to 2d20940 Compare December 6, 2023 13:18
@Praveen2112
Copy link
Copy Markdown
Member Author

@marcinsbd / @krvikash Thanks for the review. AC

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why not throw new TableNotFoundException(schemaTableName)? database and collection variables are ensured as lowercases.

@Praveen2112 Praveen2112 force-pushed the praveen/mongodb_cleanup branch from 2d20940 to f757f10 Compare December 8, 2023 06:22
@Praveen2112
Copy link
Copy Markdown
Member Author

@ebyhr Thanks a lot for the review. AC

@Praveen2112 Praveen2112 merged commit 0aa1f1b into trinodb:master Dec 11, 2023
@github-actions github-actions bot added this to the 435 milestone Dec 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla-signed mongodb MongoDB connector

Development

Successfully merging this pull request may close these issues.

4 participants