Some comments on new scheduled delivery work

[timfox]

A few comments:

a) Can you tell your formatter to add a space between if statements and the opening brace?

E.g.

if (x == 3)

not

if(x == 3)

b) Main issue. Scheduled delivery time is being stored in the journal at the message level. However, it should be an attribute of the message reference, not the message. (See my earlier post)

I.e. two different references for the same message can have different scheduled delivery times. This is the same as it works in JBM 1.4.

We need to do it this way since different subscriptions for the same topic can roll back independently with a delayed redelivery.

The current journal loading and storing code assumes the attribute is for the message.

c) Can you point me to the core tests for this functionality?

[timfox]

One other thing:

If you're going to add a scheduledDeliveryTime attribute to the ref, then there's no need for a separate addScheduledDelivery method, so they should be combined.

More over the current addScheduledDelivery method bypasses all the message and size counting done in add.

[ataylor]

a) Can you tell your formatter to add a space between if statements and the opening brace?

It already is, I musn't have formatted this piece of code.

anonymous wrote : b) Main issue. Scheduled delivery time is being stored in the journal at the message level. However, it should be an attribute of the message reference, not the message. (See my earlier post)

So at what point do i add a record to the journal then? I guess i just need to add the queue id and the message id to the encoding.

anonymous wrote : ) Can you point me to the core tests for this functionality?

theres a ScheduledMessageTest under the timing tests. Ive also added some tests to unut tests that already exist, journalManagerTest etc

If you're going to add a scheduledDeliveryTime attribute to the ref, then there's no need for a separate addScheduledDelivery method, so they should be combined.

More over the current addScheduledDelivery method bypasses all the message and size counting done in add.

Ok i'll sort

[timfox]

"ataylor" wrote:


So at what point do i add a record to the journal then? I guess i just need to add the queue id and the message id to the encoding.

Yes, the same as is currently done for acknowledgements, which are also at the message reference level as opposed to the message level.

[timfox]

It would be a good idea to add tests for routing a message to multiple queues, and testing that redelivery delay is handled separately for each reference.

[timfox]

Don't worry too much about unit tests, more important is to have the core functionality tested at the core level.

I think these are currently missing.

[ataylor]

It would be a good idea to add tests for routing a message to multiple queues, and testing that redelivery delay is handled separately for each reference.

ok

Don't worry too much about unit tests, more important is to have the core functionality tested at the core level.

This is ScheduledMessageTest but since it relies on timing i put in under the timing tests.

[ataylor]

I also would like to write a core integration test that forces paging from the off and then force a depage to make sure that the message is reloaded correctly, i could only write a unit test for this, maybe Clebert can help?

[timfox]

"ataylor" wrote:

This is ScheduledMessageTest but since it relies on timing i put in under the timing tests.

Ok, looking at the test it looks like an integration tests to me, so should live in integration/something.

I'd also add some tests which test basic scheduled functionality, single and multiple subscribers, mixture of scheduled and non scheduled messages.

Mixtures of different scheduled times etc. Take a look at the jms test for ideas.

Timing tests are basically unit tests which have long pauses etc in them, not integration tests.

[timfox]

"ataylor" wrote:
I also would like to write a core integration test that forces paging from the off and then force a depage to make sure that the message is reloaded correctly, i could only write a unit test for this, maybe Clebert can help?

Why could you only write a unit test for this? You can start the server with low values of paging surely.

[ataylor]

Why could you only write a unit test for this? You can start the server with low values of paging surely.

I wasn't sure how to force a depage, I guess i could set the value to the size of the first message sent forcing the second (scheduled) message to be paged.

[clebert.suconic]

I don't know how that would apply to your test.. the the PagingStore has a method to startPaging and startDepage.

But if you are using regular sending of messages in your test, a depage might start sooner, as the size of the page is tested on every ACK.

[clebert.suconic]

Can we avoid having a different packet and method for scheduledDeliveryTime?

I'm also creating a packet for Big chunks... and the combination of this with ScheduledDeliveryTime would be somewhat weird.

Bigchunk & scheduled
BigChunk & nonscheduled
RegularSize & scheduled
RegularSize & nonScheduled

If you (Andy) is done with scheduledDelivery, I could take care of the change (if you agree with the change of course).

[ataylor]

Ive now changed this so the journal stores against the reference. Ive also added some integration tests.