Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OGM-1501 Add support for ReadConcern #1060

Closed

Conversation

@Krerg
Copy link
Contributor

Krerg commented Jul 5, 2018

No description provided.

@@ -862,8 +881,11 @@ public void forEachTuple(ModelConsumer consumer, TupleTypeContext tupleTypeConte
}

String collectionName = getCollectionName( backendQuery, queryDescriptor, entityKeyMetadata );
//WriteConcern writeConcern = getWriteConcern( tupleContext );
//ReadPreference readPreference = getReadPreference( tupleContext );

This comment has been minimized.

Copy link
@DavideD

DavideD Jul 9, 2018

Member

THere shouldn't be commented code, you can delete it's not necessary anymore

MongoCollection<Document> collection = provider.getDatabase().getCollection( collectionName );

This comment has been minimized.

Copy link
@DavideD

DavideD Jul 9, 2018

Member

How come you area not applying the withOption method in this case? You are not applying anymore the write or read options, I'm surprised that no test fails in because of this

This comment has been minimized.

Copy link
@Krerg

Krerg Jul 9, 2018

Author Contributor

Applying options when executing backend query is OGM-1448 bug. In tests only getObject, getObjects and getAssociation are called

This comment has been minimized.

Copy link
@DavideD

DavideD Jul 9, 2018

Member

Mmmh... OK

@Krerg Krerg force-pushed the epam:feature/OGM-1501-support-read-concern-dev branch from 34b45f2 to 6f485a1 Jul 9, 2018
Copy link
Member

DavideD left a comment

The approach looks fine but I think it needs some changes:

  • the method MongoBDDialect#executeBackendQuery doesn't apply the options anymore in this PR but it did before;
  • we should apply the options when reading the collection in MongoBDDialect#forEachTuple;
  • we should test that the options are correct when using MonogBDDialect#executeBackendQuery and MongoBDDialect#forEachTuple;

It would be nice if, instead of having a single commit, you could send a PR with separate logical commits, for example the classes and the tests could be in separate commits.

Thanks a lot

@@ -415,6 +418,21 @@ private static Object prepareIdObjectValue(String[] columnNames, Object[] column
return getCollection( key.getTable() );
}

private MongoCollection<Document> withOptions(MongoCollection<Document> collection, ReadPreference readPreference, ReadConcern readConcern) {
if ( readPreference != null && readConcern != null ) {

This comment has been minimized.

Copy link
@DavideD

DavideD Jul 9, 2018

Member

I think this method should also apply the writeConcern and should be used everytime with need a collection.
This way the options are applied always by the same function.

I also find this code harder to read than needed, what do you think about refactoring it to look something like:

	private MongoCollection<Document> withOptions(MongoCollection<Document> collection, AssociationContext context) {
		return withOptions( collection, context.getAssociationTypeContext().getOptionsContext() );
	}

	private MongoCollection<Document> withOptions(MongoCollection<Document> collection, TupleTypeContext context) {
		return withOptions( collection, context.getOptionsContext() );
	}

	private MongoCollection<Document> withOptions(MongoCollection<Document> collection, OptionsContext context) {
		MongoCollection<Document> newCollection = collection;
		newCollection = withReadPreference( context, newCollection );
		newCollection = withReadConcern( context, newCollection );
		return newCollection;
	}

	private MongoCollection<Document> withReadConcern(OptionsContext context, MongoCollection<Document> newCollection) {
		ReadConcern readConcern = context.getUnique( ReadConcernOption.class );
		if ( readConcern != null ) {
			newCollection = newCollection.withReadConcern( readConcern );
		}
		return newCollection;
	}

	private MongoCollection<Document> withReadPreference(OptionsContext context, MongoCollection<Document> newCollection) {
		ReadPreference readPreference = context.getUnique( ReadPreferenceOption.class );
		if ( readPreference != null ) {
			return newCollection.withReadPreference( readPreference );
		}
		return newCollection;
	}

Now that think about it it might make sense to call withOptions everytime we call the getCollection( ),
something like:

	private MongoCollection<Document> getCollection(String table, ...) {
		return withOptions( currentDB.getCollection( table ), ... );
	}

This comment has been minimized.

Copy link
@DavideD

DavideD Jul 9, 2018

Member

for example the classes and the tests could be in separate commits.

What I meant is the new classes and the new tests could be in their own commit

Aleksandr Mylnikov added 2 commits Jul 17, 2018
 - Refactor solution
 - Always apply options when getting collection in MongoDb dialect
@Krerg

This comment has been minimized.

Copy link
Contributor Author

Krerg commented Jul 24, 2018

@DavideD, Changes are ready to review

return tupleContext.getTupleTypeContext().getOptionsContext();
}
catch (HibernateException exception) {
return null;

This comment has been minimized.

Copy link
@DavideD

DavideD Jul 26, 2018

Member

What kind of exception occurs here that you can ignore?

This comment has been minimized.

Copy link
@Krerg

Krerg Jul 27, 2018

Author Contributor

There could be OnlyWithTransactionContext which throws exception with message "The tuple context is not available, probably because we are dealing with more than a single entity type".

*/
LINEARIZABLE(ReadConcern.LINEARIZABLE);

private static Log log = LoggerFactory.make( MethodHandles.lookup() );

This comment has been minimized.

Copy link
@DavideD

DavideD Jul 26, 2018

Member

Log is not used anywhere, you can remove it

Document dbObject = objectForInsert( tuple, ( (MongoDBTupleSnapshot) tuple.getSnapshot() ).getDbObject() );
getCollection( entityKeyMetadata ).withWriteConcern( writeConcern ).insertOne( dbObject );
getCollection( entityKeyMetadata.getTable() , tupleContext.getTupleTypeContext().getOptionsContext() ).withWriteConcern( writeConcern ).insertOne( dbObject );

This comment has been minimized.

Copy link
@DavideD

DavideD Jul 26, 2018

Member

Why don't you set the writeConcern in the getCollection method so that it is set every time getCollection is called?
Basically, the same thing we do for readConcern

This comment has been minimized.

Copy link
@DavideD

DavideD Jul 26, 2018

Member

By the way, there is a space after getTable: should be .getTable(), and not .getTable() ,

@DavideD

This comment has been minimized.

Copy link
Member

DavideD commented Aug 1, 2018

Merged with some changes #1060

@DavideD DavideD closed this Aug 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.