camel-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jon Woodforth (JIRA)" <j...@apache.org>
Subject [jira] [Comment Edited] (CAMEL-10340) camel-aws - SQS option deleteAfterRead not work if set deleteIfFiltered=false
Date Wed, 23 Nov 2016 09:15:00 GMT

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

Jon Woodforth edited comment on CAMEL-10340 at 11/23/16 9:14 AM:
-----------------------------------------------------------------

Hi,

As per comments on the commit here:  https://github.com/apache/camel/commit/50dc4ea14c0ad6d3d93cfe4ddb386a006d1fafcc

This change has changed the behaviour of our routes and deviates from the documentation. Can
I ask if this is what was intended? ...this doesn't seem to match what was in the Camel-SQS
documentation.

The code before was:

{code}
    private boolean shouldDelete(Exchange exchange) {
        return getConfiguration().isDeleteAfterRead()
                && (getConfiguration().isDeleteIfFiltered()
                    || (!getConfiguration().isDeleteIfFiltered()
                        && passedThroughFilter(exchange)));
    }
{code}

Which matched the documentation:

deleteAfterRead: Delete message from SQS after it has been read (and processed by the route).
deleteIfFiltered: If false and exchange does not make it through a Camel filter upstream in
the route, then don't send DeleteMessage.

With the above code, deleteAfterRead needed to be set to "true" for ANY message to be deleted,
filtered or not. Then, it was up to the configuration of deleteIfFiltered - if this was set
to false then as the documentation states the message would not be deleted if if didn't pass
through the filter.

So, our config of:
deleteAfterRead = true (default)
deleteIfFiltered = false

...gave the right outcome. Messages that were filtered "out" (i.e. not allowed through the
filter) were not deleted from SQS.

However, the new code commit has changed this behaviour.

Firstly:

{code}
return getConfiguration().isDeleteAfterRead() && (...filteringLogic...)
{code}

..has changed to:

{code}
return getConfiguration().isDeleteAfterRead() || shouldDeleteByFilter;
{code}

.. an && becoming an || which means for the filtering logic to take effect the deleteAfterRead
must not be set to false.  This isn't covered in the documentation and is a change of behaviour
from before.

Secondly:

{code}
|| (!getConfiguration().isDeleteIfFiltered() && passedThroughFilter(exchange)));
{code}

..has changed to:

{code}
... && getConfiguration().isDeleteIfFiltered() && passedThroughFilter(exchange);
{code}

.. this time the negation has been removed, so the logic has now changed to not looking at
whether the message was filtered out, but whether is was allowed through the filter.

With the new code that has been committed, to get the same behaviour as before you now have
to set deleteAfterRead to "false" and deletedIfFiltered to "true" so that messages filtered
out are not deleted, and those that pass the filter are.

New config:
deleteAfterRead = false
deleteIfFiltered = true

Is this what was intended? Seems to be a departure from the documentation? If so is the documentation
going to be updated? ..or should the logic be put back how it was?

I believe, reading the posts above, that the code worked the way it was; if deleteAfterRead
was set to true and deleteIfFiltered was set to false then the scenarios above would have
worked - messages that did not pass the filter would not have been deleted; those that did
pass the filter would have been.  I don't see why the code was changed!


was (Author: jonw9):
Hi,

As per comments on the commit here:  https://github.com/apache/camel/commit/50dc4ea14c0ad6d3d93cfe4ddb386a006d1fafcc

This change has changed the behaviour of our routes and deviates from the documentation. Can
I ask if this is what was intended? ...this doesn't seem to match what was in the Camel-SQS
documentation.

The code before was:

    private boolean shouldDelete(Exchange exchange) {
        return getConfiguration().isDeleteAfterRead()
                && (getConfiguration().isDeleteIfFiltered()
                    || (!getConfiguration().isDeleteIfFiltered()
                        && passedThroughFilter(exchange)));
    }

Which matched the documentation:

deleteAfterRead: Delete message from SQS after it has been read (and processed by the route).
deleteIfFiltered: If false and exchange does not make it through a Camel filter upstream in
the route, then don't send DeleteMessage.

With the above code, deleteAfterRead needed to be set to "true" for ANY message to be deleted,
filtered or not. Then, it was up to the configuration of deleteIfFiltered - if this was set
to false then as the documentation states the message would not be deleted if if didn't pass
through the filter.

So, our config of:
deleteAfterRead = true (default)
deleteIfFiltered = false

...gave the right outcome. Messages that were filtered "out" (i.e. not allowed through the
filter) were not deleted from SQS.

However, the new code commit has changed this behaviour.

Firstly:

{code}
return getConfiguration().isDeleteAfterRead() && (...filteringLogic...)
{code}

..has changed to:

{code}
return getConfiguration().isDeleteAfterRead() || shouldDeleteByFilter;
{code}

.. an && becoming an || which means for the filtering logic to take effect the deleteAfterRead
must not be set to false.  This isn't covered in the documentation and is a change of behaviour
from before.

Secondly:

{code}
|| (!getConfiguration().isDeleteIfFiltered() && passedThroughFilter(exchange)));
{code}

..has changed to:

{code}
... && getConfiguration().isDeleteIfFiltered() && passedThroughFilter(exchange);
{code}

.. this time the negation has been removed, so the logic has now changed to not looking at
whether the message was filtered out, but whether is was allowed through the filter.

With the new code that has been committed, to get the same behaviour as before you now have
to set deleteAfterRead to "false" and deletedIfFiltered to "true" so that messages filtered
out are not deleted, and those that pass the filter are.

New config:
deleteAfterRead = false
deleteIfFiltered = true

Is this what was intended? Seems to be a departure from the documentation? If so is the documentation
going to be updated? ..or should the logic be put back how it was?

I believe, reading the posts above, that the code worked the way it was; if deleteAfterRead
was set to true and deleteIfFiltered was set to false then the scenarios above would have
worked - messages that did not pass the filter would not have been deleted; those that did
pass the filter would have been.  I don't see why the code was changed!

> camel-aws - SQS option deleteAfterRead not work if set deleteIfFiltered=false
> -----------------------------------------------------------------------------
>
>                 Key: CAMEL-10340
>                 URL: https://issues.apache.org/jira/browse/CAMEL-10340
>             Project: Camel
>          Issue Type: Bug
>          Components: camel-aws
>    Affects Versions: 2.17.3
>            Reporter: Yi Yan
>            Assignee: Andrea Cosentino
>            Priority: Minor
>             Fix For: 2.18.0, 2.17.4
>
>         Attachments: SqsConsumerDeleteTest.java
>
>
> I'm using aws-sqs 2.17.3, if I set deleteAfterRead=true and deleteIfFiltered=false in
my DSL, the message will not be deleted. If I want to delete the message after read it, I
have to set deleteAfterRead and deleteIfFiltered both with true when I use the two options
in one DSL, but in fact there is no filter in my route, the message should be removed whatever
the deleteIfFiltered option set to ture or false.
> {code:title=SqsConsumerDeleteTest.java|borderStyle=solid}
> from("aws-sqs:my-quque"
>     + "?amazonSQSClient=#conn_cAWSConnection_1"
>     + "&deleteAfterRead=" + true + "&deleteIfFiltered="
>     + false).to("log:qs_route.cLog_1" + "?level=DEBUG").to("mock:mock_1");
> {code}
> I attached my test file, after run the test method, the sqs message still exists in the
sqs queue after 30 seconds.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Mime
View raw message