Skip to content

Conversation

artembilan
Copy link
Member

Copy link
Member Author

Choose a reason for hiding this comment

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

@garyrussell
Copy link
Contributor

I see no issues so far but I agree we need a sequence.

I am not sure what you mean about GC, the query strings look like literals to me.

Maybe we can add ALTER TABLE scripts, to ease migration?

@artembilan
Copy link
Member Author

you mean about GC

I mean my replaceFirst hack. However, Now I see a case, where I'm not right: the custom ChannelMessageStoreQueryProvider might not provide CREATED_DATE ASC clause and that my hack will be volnurability. So, I'll add new strategy methods to ChannelMessageStoreQueryProvider.

ALTER TABLE scripts, to ease migration

Actually it's simple task for DBA: to compare initial scripts from different version and write ALTER TABLE (and others) scripts. I'd say: let's provide them, but on demand!

@garyrussell
Copy link
Contributor

I am ok with that, but let's add a note in the migration guide about the new columns added.

@artembilan
Copy link
Member Author

Pushed entire priority stuff.
Tested on MySQL, too.
Still need sequence and Docs

@artembilan
Copy link
Member Author

Pushed sequence stuff

@artembilan
Copy link
Member Author

Pushed Docs.

All work is done. The Migration Guide note will be added after merge.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you need to promote this method? ChannelMessageStores don't need to remove their groups.

Copy link
Member Author

Choose a reason for hiding this comment

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

No, It's really useful - purge in terms of AMQP

@garyrussell
Copy link
Contributor

That's all

Copy link
Contributor

Choose a reason for hiding this comment

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

We also need to state that you can't use the same message store for both queue and priority channels (even with different regions) because the priority setting applies to the entire store.

Using a priorityEnabled store with a QueueChannel would not produce the expected results.

@artembilan
Copy link
Member Author

Pushed

Copy link
Contributor

Choose a reason for hiding this comment

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

Fails on Oracle

java.lang.AssertionError: 
Expected: an instance of java.lang.Long
     but: <1> is a java.math.BigDecimal
    at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:20)
    at org.junit.Assert.assertThat(Assert.java:865)
    at org.junit.Assert.assertThat(Assert.java:832)
    at org.springframework.integration.jdbc.store.channel.AbstractTxTimeoutMessageStoreTests.testMessageSequenceColumn(AbstractTxTimeoutMessageStoreTests.java:221)

Copy link
Member Author

Choose a reason for hiding this comment

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

true. Can we compare just two Number ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, this works...

garyrussell@3515f95

@garyrussell
Copy link
Contributor

LGTM; final polish here: garyrussell@a7f36af

@garyrussell
Copy link
Contributor

Merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants