13 Replies Latest reply on Oct 9, 2008 5:27 AM by Andy Taylor

    Some comments on new scheduled delivery work

    Tim Fox Master

      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?

        • 1. Re: Some comments on new scheduled delivery work
          Tim Fox Master

          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.

          • 2. Re: Some comments on new scheduled delivery work
            Andy Taylor Master

             

            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


            • 3. Re: Some comments on new scheduled delivery work
              Tim Fox Master

               

              "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.

              • 4. Re: Some comments on new scheduled delivery work
                Tim Fox Master

                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.

                • 5. Re: Some comments on new scheduled delivery work
                  Tim Fox Master

                  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.

                  • 6. Re: Some comments on new scheduled delivery work
                    Andy Taylor Master

                     

                    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.

                    • 7. Re: Some comments on new scheduled delivery work
                      Andy Taylor Master

                      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?

                      • 8. Re: Some comments on new scheduled delivery work
                        Tim Fox Master

                         

                        "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.

                        • 9. Re: Some comments on new scheduled delivery work
                          Tim Fox Master

                           

                          "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.

                          • 10. Re: Some comments on new scheduled delivery work
                            Andy Taylor Master

                             

                            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.

                            • 11. Re: Some comments on new scheduled delivery work
                              Clebert Suconic Master

                              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.

                              • 12. Re: Some comments on new scheduled delivery work
                                Clebert Suconic Master

                                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).

                                • 13. Re: Some comments on new scheduled delivery work
                                  Andy Taylor Master

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