Skip to content

INT-3337: MongoDbMessageStore refactoring #1091

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 6 commits into from
Closed

INT-3337: MongoDbMessageStore refactoring #1091

wants to merge 6 commits into from

Conversation

@artembilan
Copy link
Member

@artembilan artembilan commented Mar 24, 2014

JIRA: https://jira.spring.io/browse/INT-3337

  • Introduce MessageReadingMongoConverter (similar to MongoDbMessageStore.MessageReadingMongoConverter)
  • Change tests to use ConfigurableMongoDbMessageStore with MessageReadingMongoConverter

garyrussell
garyrussell reviewed Mar 25, 2014
View reviewed changes
...mongodb/src/main/java/org/springframework/integration/mongodb/store/MongoDbMessageStore.java Outdated
Copy link
Contributor

@garyrussell garyrussell Mar 25, 2014

Choose a reason for hiding this comment

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

I am not so sure the rename will be necessary.

Copy link
Member Author

@artembilan artembilan Mar 27, 2014

Choose a reason for hiding this comment

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

Why? My position to remove MongoDbMessageStore as redundant.
And when we will have just only ConfigurableMongoDbMessageStore, there might be a question: "And where is a plain MongoDbMessageStore? What is a reason to call it as 'Configurable', if there is no other implementation ?"

Copy link
Contributor

@garyrussell garyrussell Mar 27, 2014

Choose a reason for hiding this comment

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

I just don't see any compelling reason to inflict any pain on users; assuming they take heed of the deprecation, they will change their apps to use the configurable store; why would we force them to do another rename late? It just seems unnecessary to me.

I am not terribly concerned either way (whether we decide to do it or not), but I see no reason to include the ...will be renamed... in the JavaDocs.

Copy link
Member Author

@artembilan artembilan Mar 27, 2014

Choose a reason for hiding this comment

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

OK. I won't mind to leave it as is and don't rename.
If it is only one concern here, I'll push the fix soon.

Copy link
Contributor

@garyrussell garyrussell Mar 27, 2014

Choose a reason for hiding this comment

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

I didn't finish my review yet; let me take another look this morning.

@garyrussell
Copy link
Contributor

garyrussell commented Mar 27, 2014

Otherwise LGTM

@artembilan
Copy link
Member Author

artembilan commented Mar 27, 2014

Pushed

1 similar comment
@artembilan
Copy link
Member Author

artembilan commented Mar 27, 2014

Pushed

garyrussell
garyrussell reviewed Mar 27, 2014
View reviewed changes
...c/main/java/org/springframework/integration/mongodb/store/support/MessageDocumentFields.java Outdated
Copy link
Contributor

@garyrussell garyrussell Mar 27, 2014

Choose a reason for hiding this comment

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

private ctor() ?

Copy link
Member Author

@artembilan artembilan Mar 27, 2014

Choose a reason for hiding this comment

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

Sure!

@artembilan
Copy link
Member Author

artembilan commented Mar 28, 2014

Pushed as we discussed privately:

  • Revert any changes
  • move *Message converters to the MongoDbMessageStore
  • Add tests

Artem Bilan added 5 commits March 28, 2014 17:35
INT-3337: MongoDbMessageStore refactoring
b5db27e 
JIRA: https://jira.spring.io/browse/INT-3337

* Introduce `MessageReadingMongoConverter` (similar to `MongoDbMessageStore.MessageReadingMongoConverter`)
* Change tests to use `ConfigurableMongoDbMessageStore` with `MessageReadingMongoConverter`
INT-3337: Addressing PR comments
9ecea33
INT-3337: Add support to store/read Messages
2e69e5c 
* Add for `MessageDocumentMongoConverter` custom `converters` for all known `Message<?>` type
* Add `MessageDocumentFields`
* Add support to store/read `ErrorMessage`. As a trick for `Throwable` is selected (de)serializing converter
INT-3337: Add converters for Message<?> impls
4b4cb8f
INT-3337: Make MDbMS.MessageWrapper AuditAware
11084c5 
Add `_id` persistence field
@artembilan
Copy link
Member Author

artembilan commented Mar 28, 2014

Pushed _id for MessageWrapper and ACAware for MMS

garyrussell
garyrussell reviewed Mar 28, 2014
View reviewed changes
...mongodb/src/main/java/org/springframework/integration/mongodb/store/MongoDbMessageStore.java
Copy link
Contributor

@garyrussell garyrussell Mar 28, 2014

Choose a reason for hiding this comment

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

Maybe we should go ahead and rename as we discussed before MessageDocumentMongoConverter.

Copy link
Member Author

@artembilan artembilan Mar 28, 2014

Choose a reason for hiding this comment

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

Keep in mind here it has name MessageWrapper and we can't change the name for backward compatibility.
From other side MessageReadingMongoConverter is an inner class, so its name doesn't have value too.

Copy link
Contributor

@garyrussell garyrussell Mar 28, 2014

Choose a reason for hiding this comment

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

np;

@garyrussell
Copy link
Contributor

garyrussell commented Mar 28, 2014

Need to update the docs to reflect the differences between the 2 stores; currently

The MongoDbMessageStore remains for backward compatibility and may be removed in future releases.

@artembilan
Copy link
Member Author

artembilan commented Mar 28, 2014

But we have in doc a difference comment. However I won't mind to add the note regarding removal in the future.

@artembilan
Copy link
Member Author

artembilan commented Mar 28, 2014

Pushed

garyrussell
garyrussell reviewed Mar 28, 2014
View reviewed changes
...main/java/org/springframework/integration/mongodb/store/ConfigurableMongoDbMessageStore.java
Copy link
Contributor

@garyrussell garyrussell Mar 28, 2014

Choose a reason for hiding this comment

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

Do you get a warning for this? Eclipse doesn't complain when the @Id annotation is present.

Copy link
Member Author

@artembilan artembilan Mar 28, 2014

Choose a reason for hiding this comment

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

Yes, IDEA complains

Copy link
Contributor

@garyrussell garyrussell Mar 28, 2014

Choose a reason for hiding this comment

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

OK; LGTM; merging.

@garyrussell
Copy link
Contributor

garyrussell commented Mar 28, 2014

Merging with these fixes.

@garyrussell
Copy link
Contributor

garyrussell commented Mar 28, 2014

Merged

@artembilan artembilan closed this Mar 28, 2014
@artembilan artembilan deleted the INT-3337 branch April 25, 2014 15:01
@spring-operator spring-operator mentioned this pull request Mar 31, 2014
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@artembilan @garyrussell