[jira] Created: (CAMEL-1159) Check the logic in Aggregator.isBatchCompleted()

classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|

[jira] Created: (CAMEL-1159) Check the logic in Aggregator.isBatchCompleted()

JIRA jira@apache.org
Check the logic in Aggregator.isBatchCompleted()
------------------------------------------------

                 Key: CAMEL-1159
                 URL: https://issues.apache.org/activemq/browse/CAMEL-1159
             Project: Apache Camel
          Issue Type: Task
          Components: camel-core
            Reporter: William Tam
            Assignee: William Tam
             Fix For: 2.0.0


Understand the logic of this method and also see if the protected method getCollection() is really needed (or getCollectionSize() is suffice).  The reason being supporting the getCollection() method may constraint the BatchProcessor class to implement less efficient algorithm.

{code}
    @Override
    protected boolean isBatchCompleted(int index) {
        if (aggregationCompletedPredicate != null) {
            // TODO: (davsclaus) What is the point with this code? I think its wrong
            if (getCollection().size() > 0) {
                return true;
            }
        }

        return super.isBatchCompleted(index);
    }
{code}

--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.

Reply | Threaded
Open this post in threaded view
|

[jira] Work started: (CAMEL-1159) Check the logic in Aggregator.isBatchCompleted()

JIRA jira@apache.org

     [ https://issues.apache.org/activemq/browse/CAMEL-1159?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Work on CAMEL-1159 started by William Tam.

> Check the logic in Aggregator.isBatchCompleted()
> ------------------------------------------------
>
>                 Key: CAMEL-1159
>                 URL: https://issues.apache.org/activemq/browse/CAMEL-1159
>             Project: Apache Camel
>          Issue Type: Task
>          Components: camel-core
>            Reporter: William Tam
>            Assignee: William Tam
>             Fix For: 2.0.0
>
>
> Understand the logic of this method and also see if the protected method getCollection() is really needed (or getCollectionSize() is suffice).  The reason being supporting the getCollection() method may constraint the BatchProcessor class to implement less efficient algorithm.
> {code}
>     @Override
>     protected boolean isBatchCompleted(int index) {
>         if (aggregationCompletedPredicate != null) {
>             // TODO: (davsclaus) What is the point with this code? I think its wrong
>             if (getCollection().size() > 0) {
>                 return true;
>             }
>         }
>         return super.isBatchCompleted(index);
>     }
> {code}

--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.

Reply | Threaded
Open this post in threaded view
|

[jira] Commented: (CAMEL-1159) Check the logic in Aggregator.isBatchCompleted()

JIRA jira@apache.org
In reply to this post by JIRA jira@apache.org

    [ https://issues.apache.org/activemq/browse/CAMEL-1159?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=48169#action_48169 ]

William Tam commented on CAMEL-1159:
------------------------------------

I found that BatchProcessor did not implement the support for "outBatchSize" properly.  I applied a fix and method in question (Aggregator.isBatchCompleted) is not needed as a result.  Basically, the isInBatchCompleted() checks for whether the in queue should be drained to the out collection based on the batchSize parameter.  I added a method "isOutBatchCompleted()" to check whether the out collection should be sent based on the outBatchSize parameter.

> Check the logic in Aggregator.isBatchCompleted()
> ------------------------------------------------
>
>                 Key: CAMEL-1159
>                 URL: https://issues.apache.org/activemq/browse/CAMEL-1159
>             Project: Apache Camel
>          Issue Type: Task
>          Components: camel-core
>            Reporter: William Tam
>            Assignee: William Tam
>             Fix For: 2.0.0
>
>
> Understand the logic of this method and also see if the protected method getCollection() is really needed (or getCollectionSize() is suffice).  The reason being supporting the getCollection() method may constraint the BatchProcessor class to implement less efficient algorithm.
> {code}
>     @Override
>     protected boolean isBatchCompleted(int index) {
>         if (aggregationCompletedPredicate != null) {
>             // TODO: (davsclaus) What is the point with this code? I think its wrong
>             if (getCollection().size() > 0) {
>                 return true;
>             }
>         }
>         return super.isBatchCompleted(index);
>     }
> {code}

--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.

Reply | Threaded
Open this post in threaded view
|

[jira] Commented: (CAMEL-1159) Check the logic in Aggregator.isBatchCompleted()

JIRA jira@apache.org
In reply to this post by JIRA jira@apache.org

    [ https://issues.apache.org/activemq/browse/CAMEL-1159?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=48172#action_48172 ]

Claus Ibsen commented on CAMEL-1159:
------------------------------------

Hi William

The OUT batch size was something I added recently for the aggregator, needed by end-users.

Martin Krasser (contributer with great patches for aggregator, resequencer, splitter etc.) said he would create a patch with the IN / OUT batch options in his xxxConfig objects (etc StreamResequencerConfig).

So we might need to check the wiki
- splitter
- resequencer
- aggregator

That the batch size options is documented. See aggregator that has this documentation (batch options). So if the splitter and resequencer now also supports the OUT batch size then we should copy the documentation (batch options) to their wiki page as well.

And maybe the OUT batch size should be exposed on
- the fluent builders
- the xxxConfig objects (StreamResequencerConfig and the other one)

> Check the logic in Aggregator.isBatchCompleted()
> ------------------------------------------------
>
>                 Key: CAMEL-1159
>                 URL: https://issues.apache.org/activemq/browse/CAMEL-1159
>             Project: Apache Camel
>          Issue Type: Task
>          Components: camel-core
>            Reporter: William Tam
>            Assignee: William Tam
>             Fix For: 2.0.0
>
>
> Understand the logic of this method and also see if the protected method getCollection() is really needed (or getCollectionSize() is suffice).  The reason being supporting the getCollection() method may constraint the BatchProcessor class to implement less efficient algorithm.
> {code}
>     @Override
>     protected boolean isBatchCompleted(int index) {
>         if (aggregationCompletedPredicate != null) {
>             // TODO: (davsclaus) What is the point with this code? I think its wrong
>             if (getCollection().size() > 0) {
>                 return true;
>             }
>         }
>         return super.isBatchCompleted(index);
>     }
> {code}

--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.

Reply | Threaded
Open this post in threaded view
|

[jira] Commented: (CAMEL-1159) Check the logic in Aggregator.isBatchCompleted()

JIRA jira@apache.org
In reply to this post by JIRA jira@apache.org

    [ https://issues.apache.org/activemq/browse/CAMEL-1159?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=48182#action_48182 ]

William Tam commented on CAMEL-1159:
------------------------------------

Thanks Claus.   The outBatchSize for Aggregator can be set via fluent builder and spring per wiki currently but that is not the problem thought.  (I'll check resequencer and splitter.  splitter does not extend BatchProcessor, tho).  Also, it is orthogonal to the xxxConfig works.  What I saw was:

* Aggregator.isBatchCompleted() does not take the outBatchSize into the account.  The method returns true if aggregationCompletedPredicate and collection.size() > 0.  So, the incoming exchanges are correctly batched and stored the queue but the "out collection" is not batched.

* The BatchProcessor.isBatchCompleted() method checks for outBatchSize first and then ignore "in" batchSize if it is true.   So, it could drain the in queue prematurely (i.e. drain the quene before batchSize is > num).  I think we should have two methods: isInBatchCompleted() and isOutBatchCompleted().  If isInBatchCompleted(), then it should drain the exchanges from in queue to out collection.  if isOutBatchCompleted(), then we should sent out the exchanges.

I attached the patch.   Let me know if I am missing something.  I still have fix up the spring test.




> Check the logic in Aggregator.isBatchCompleted()
> ------------------------------------------------
>
>                 Key: CAMEL-1159
>                 URL: https://issues.apache.org/activemq/browse/CAMEL-1159
>             Project: Apache Camel
>          Issue Type: Task
>          Components: camel-core
>            Reporter: William Tam
>            Assignee: William Tam
>             Fix For: 2.0.0
>
>
> Understand the logic of this method and also see if the protected method getCollection() is really needed (or getCollectionSize() is suffice).  The reason being supporting the getCollection() method may constraint the BatchProcessor class to implement less efficient algorithm.
> {code}
>     @Override
>     protected boolean isBatchCompleted(int index) {
>         if (aggregationCompletedPredicate != null) {
>             // TODO: (davsclaus) What is the point with this code? I think its wrong
>             if (getCollection().size() > 0) {
>                 return true;
>             }
>         }
>         return super.isBatchCompleted(index);
>     }
> {code}

--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.

Reply | Threaded
Open this post in threaded view
|

[jira] Updated: (CAMEL-1159) Check the logic in Aggregator.isBatchCompleted()

JIRA jira@apache.org
In reply to this post by JIRA jira@apache.org

     [ https://issues.apache.org/activemq/browse/CAMEL-1159?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

William Tam updated CAMEL-1159:
-------------------------------

    Attachment: CAMEL-1159.patch

> Check the logic in Aggregator.isBatchCompleted()
> ------------------------------------------------
>
>                 Key: CAMEL-1159
>                 URL: https://issues.apache.org/activemq/browse/CAMEL-1159
>             Project: Apache Camel
>          Issue Type: Task
>          Components: camel-core
>            Reporter: William Tam
>            Assignee: William Tam
>             Fix For: 2.0.0
>
>         Attachments: CAMEL-1159.patch
>
>
> Understand the logic of this method and also see if the protected method getCollection() is really needed (or getCollectionSize() is suffice).  The reason being supporting the getCollection() method may constraint the BatchProcessor class to implement less efficient algorithm.
> {code}
>     @Override
>     protected boolean isBatchCompleted(int index) {
>         if (aggregationCompletedPredicate != null) {
>             // TODO: (davsclaus) What is the point with this code? I think its wrong
>             if (getCollection().size() > 0) {
>                 return true;
>             }
>         }
>         return super.isBatchCompleted(index);
>     }
> {code}

--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.

Reply | Threaded
Open this post in threaded view
|

[jira] Commented: (CAMEL-1159) Check the logic in Aggregator.isBatchCompleted()

JIRA jira@apache.org
In reply to this post by JIRA jira@apache.org

    [ https://issues.apache.org/activemq/browse/CAMEL-1159?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=48189#action_48189 ]

Claus Ibsen commented on CAMEL-1159:
------------------------------------

William nice patch.

I was wondering if the out batch sample using
- in batch size = 1
- out batch size = 10

<aggregate strategyRef="myAggregatorStrategy" batchSize="1" outBatchSize="10">

Why is the in batch at 1? Is it intended. Can it be removed, so you can choose which batch size you want to use
- IN (number of exchanges) received
- OUT (number of exchanges) to send

So I think a more common use case would be having higher IN batch size than OUT. If that is the fact I think we should change the sample to reflect this. There is also a Java DSL sample.

Minor spelling
// out batch is disable, so go ahead and send.
It should be *disabled*

> Check the logic in Aggregator.isBatchCompleted()
> ------------------------------------------------
>
>                 Key: CAMEL-1159
>                 URL: https://issues.apache.org/activemq/browse/CAMEL-1159
>             Project: Apache Camel
>          Issue Type: Task
>          Components: camel-core
>            Reporter: William Tam
>            Assignee: William Tam
>             Fix For: 2.0.0
>
>         Attachments: CAMEL-1159.patch
>
>
> Understand the logic of this method and also see if the protected method getCollection() is really needed (or getCollectionSize() is suffice).  The reason being supporting the getCollection() method may constraint the BatchProcessor class to implement less efficient algorithm.
> {code}
>     @Override
>     protected boolean isBatchCompleted(int index) {
>         if (aggregationCompletedPredicate != null) {
>             // TODO: (davsclaus) What is the point with this code? I think its wrong
>             if (getCollection().size() > 0) {
>                 return true;
>             }
>         }
>         return super.isBatchCompleted(index);
>     }
> {code}

--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.

Reply | Threaded
Open this post in threaded view
|

[jira] Updated: (CAMEL-1159) Check the logic in Aggregator.isBatchCompleted()

JIRA jira@apache.org
In reply to this post by JIRA jira@apache.org

     [ https://issues.apache.org/activemq/browse/CAMEL-1159?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

William Tam updated CAMEL-1159:
-------------------------------

    Attachment: CAMEL-1159.patch

> Check the logic in Aggregator.isBatchCompleted()
> ------------------------------------------------
>
>                 Key: CAMEL-1159
>                 URL: https://issues.apache.org/activemq/browse/CAMEL-1159
>             Project: Apache Camel
>          Issue Type: Task
>          Components: camel-core
>            Reporter: William Tam
>            Assignee: William Tam
>             Fix For: 2.0.0
>
>         Attachments: CAMEL-1159.patch
>
>
> Understand the logic of this method and also see if the protected method getCollection() is really needed (or getCollectionSize() is suffice).  The reason being supporting the getCollection() method may constraint the BatchProcessor class to implement less efficient algorithm.
> {code}
>     @Override
>     protected boolean isBatchCompleted(int index) {
>         if (aggregationCompletedPredicate != null) {
>             // TODO: (davsclaus) What is the point with this code? I think its wrong
>             if (getCollection().size() > 0) {
>                 return true;
>             }
>         }
>         return super.isBatchCompleted(index);
>     }
> {code}

--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.

Reply | Threaded
Open this post in threaded view
|

[jira] Updated: (CAMEL-1159) Check the logic in Aggregator.isBatchCompleted()

JIRA jira@apache.org
In reply to this post by JIRA jira@apache.org

     [ https://issues.apache.org/activemq/browse/CAMEL-1159?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

William Tam updated CAMEL-1159:
-------------------------------

    Attachment:     (was: CAMEL-1159.patch)

> Check the logic in Aggregator.isBatchCompleted()
> ------------------------------------------------
>
>                 Key: CAMEL-1159
>                 URL: https://issues.apache.org/activemq/browse/CAMEL-1159
>             Project: Apache Camel
>          Issue Type: Task
>          Components: camel-core
>            Reporter: William Tam
>            Assignee: William Tam
>             Fix For: 2.0.0
>
>         Attachments: CAMEL-1159.patch
>
>
> Understand the logic of this method and also see if the protected method getCollection() is really needed (or getCollectionSize() is suffice).  The reason being supporting the getCollection() method may constraint the BatchProcessor class to implement less efficient algorithm.
> {code}
>     @Override
>     protected boolean isBatchCompleted(int index) {
>         if (aggregationCompletedPredicate != null) {
>             // TODO: (davsclaus) What is the point with this code? I think its wrong
>             if (getCollection().size() > 0) {
>                 return true;
>             }
>         }
>         return super.isBatchCompleted(index);
>     }
> {code}

--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.

Reply | Threaded
Open this post in threaded view
|

[jira] Commented: (CAMEL-1159) Check the logic in Aggregator.isBatchCompleted()

JIRA jira@apache.org
In reply to this post by JIRA jira@apache.org

    [ https://issues.apache.org/activemq/browse/CAMEL-1159?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=48199#action_48199 ]

William Tam commented on CAMEL-1159:
------------------------------------

I fixed the mis-spell.  The batch size of 1 is to allow testing the out batch size better.  I'll exclude it from the example when possible and add some more comment.  thanks for your comments.

> Check the logic in Aggregator.isBatchCompleted()
> ------------------------------------------------
>
>                 Key: CAMEL-1159
>                 URL: https://issues.apache.org/activemq/browse/CAMEL-1159
>             Project: Apache Camel
>          Issue Type: Task
>          Components: camel-core
>            Reporter: William Tam
>            Assignee: William Tam
>             Fix For: 2.0.0
>
>         Attachments: CAMEL-1159.patch
>
>
> Understand the logic of this method and also see if the protected method getCollection() is really needed (or getCollectionSize() is suffice).  The reason being supporting the getCollection() method may constraint the BatchProcessor class to implement less efficient algorithm.
> {code}
>     @Override
>     protected boolean isBatchCompleted(int index) {
>         if (aggregationCompletedPredicate != null) {
>             // TODO: (davsclaus) What is the point with this code? I think its wrong
>             if (getCollection().size() > 0) {
>                 return true;
>             }
>         }
>         return super.isBatchCompleted(index);
>     }
> {code}

--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.

Reply | Threaded
Open this post in threaded view
|

[jira] Resolved: (CAMEL-1159) Check the logic in Aggregator.isBatchCompleted()

JIRA jira@apache.org
In reply to this post by JIRA jira@apache.org

     [ https://issues.apache.org/activemq/browse/CAMEL-1159?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

William Tam resolved CAMEL-1159.
--------------------------------

       Resolution: Fixed
    Fix Version/s: 1.5.1

1.5.1 - r726941
2.0.0 - r726932

> Check the logic in Aggregator.isBatchCompleted()
> ------------------------------------------------
>
>                 Key: CAMEL-1159
>                 URL: https://issues.apache.org/activemq/browse/CAMEL-1159
>             Project: Apache Camel
>          Issue Type: Task
>          Components: camel-core
>            Reporter: William Tam
>            Assignee: William Tam
>             Fix For: 1.5.1, 2.0.0
>
>         Attachments: CAMEL-1159.patch
>
>
> Understand the logic of this method and also see if the protected method getCollection() is really needed (or getCollectionSize() is suffice).  The reason being supporting the getCollection() method may constraint the BatchProcessor class to implement less efficient algorithm.
> {code}
>     @Override
>     protected boolean isBatchCompleted(int index) {
>         if (aggregationCompletedPredicate != null) {
>             // TODO: (davsclaus) What is the point with this code? I think its wrong
>             if (getCollection().size() > 0) {
>                 return true;
>             }
>         }
>         return super.isBatchCompleted(index);
>     }
> {code}

--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.