You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@camel.apache.org by GitBox <gi...@apache.org> on 2022/04/28 12:12:58 UTC

[GitHub] [camel] orpiske opened a new pull request, #7518: CAMEL-18024: rework the last processed offset tracking

orpiske opened a new pull request, #7518:
URL: https://github.com/apache/camel/pull/7518

   <!-- Uncomment and fill this section if your PR is not trivial
   - [ ] Make sure there is a [JIRA issue](https://issues.apache.org/jira/browse/CAMEL) filed for the change (usually before you start working on it).  Trivial changes like typos do not require a JIRA issue.  Your pull request should address just this issue, without pulling in other changes.
   - [ ] Each commit in the pull request should have a meaningful subject line and body.
   - [ ] If you're unsure, you can format the pull request title like `[CAMEL-XXX] Fixes bug in camel-file component`, where you replace `CAMEL-XXX` with the appropriate JIRA issue.
   - [ ] Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
   - [ ] Run `mvn clean install -Psourcecheck` in your module with source check enabled to make sure basic checks pass and there are no checkstyle violations. A more thorough check will be performed on your pull request automatically.
   Below are the contribution guidelines:
   https://github.com/apache/camel/blob/main/CONTRIBUTING.md
   -->


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [camel] ludovic-boutros commented on pull request #7518: CAMEL-18024: rework the last processed offset tracking

Posted by GitBox <gi...@apache.org>.
ludovic-boutros commented on PR #7518:
URL: https://github.com/apache/camel/pull/7518#issuecomment-1133016320

   @orpiske the async commit process seems to be broken. The commit() function in DefaultKafkaManualAsyncCommit calls a forceCommit() function which is a synchronous.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [camel] github-actions[bot] commented on pull request #7518: CAMEL-18024: rework the last processed offset tracking

Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #7518:
URL: https://github.com/apache/camel/pull/7518#issuecomment-1112129293

   :warning: This PR changes Camel components and will be tested automatically.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [camel] orpiske commented on pull request #7518: CAMEL-18024: rework the last processed offset tracking

Posted by GitBox <gi...@apache.org>.
orpiske commented on PR #7518:
URL: https://github.com/apache/camel/pull/7518#issuecomment-1128884147

   @ludovic-boutros doing a quick research regarding your comment I noticed our async test was disabled (it runs well on my own CI, but I re-enabled it again so we have regular checks on the Apache one). 
   
   If you have any suggestion about how to modify it to fit the use case/issue you mention, please, just let us know (or send a PR). 
   
   Thanks for reviewing it/suggesting/contributing!
   
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [camel] orpiske commented on pull request #7518: CAMEL-18024: rework the last processed offset tracking

Posted by GitBox <gi...@apache.org>.
orpiske commented on PR #7518:
URL: https://github.com/apache/camel/pull/7518#issuecomment-1133553086

   > @orpiske the async commit process seems to be broken. The commit() function in DefaultKafkaManualAsyncCommit calls a forceCommit() function which is synchronous. And when fixing it using the commit() function (async with the manager), the test throw a ConcurrentModificationException(). We need to reintroduce a ConcurrentQueue storing commits which should be process by the main kafka consumer polling loop and thread.
   
   Please, can you please open a ticket for this or send a PR with your proposed changes? 
   
   The important requirement is that the concurrent queue must be isolated to only the async-related code. It should not be part of the other code (ie.: the concerns of the async manual commit code should be separated) nor should be present in any way in the KafkaConsumer, KafkaFetchRecords or Kafka*Processor code. This is important so that we can continue cleaning up and reducing the maintenance effort for this component. 
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [camel] github-actions[bot] commented on pull request #7518: CAMEL-18024: rework the last processed offset tracking

Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #7518:
URL: https://github.com/apache/camel/pull/7518#issuecomment-1112193811

   :x: Finished component verification: **1 component(s) test failed** out of 1 component(s) tested


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [camel] ludovic-boutros commented on pull request #7518: CAMEL-18024: rework the last processed offset tracking

Posted by GitBox <gi...@apache.org>.
ludovic-boutros commented on PR #7518:
URL: https://github.com/apache/camel/pull/7518#issuecomment-1129804990

   @orpiske Thank you ! I will try to find some time to make some tests with this PR and let you know. 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [camel] github-actions[bot] commented on pull request #7518: CAMEL-18024: rework the last processed offset tracking

Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #7518:
URL: https://github.com/apache/camel/pull/7518#issuecomment-1112290933

   :x: Finished component verification: **1 component(s) test failed** out of 4 component(s) tested


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [camel] orpiske commented on pull request #7518: CAMEL-18024: rework the last processed offset tracking

Posted by GitBox <gi...@apache.org>.
orpiske commented on PR #7518:
URL: https://github.com/apache/camel/pull/7518#issuecomment-1112378197

   The checkstyle issue is not on this PR code, so it's safe to merge. 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [camel] github-actions[bot] commented on pull request #7518: CAMEL-18024: rework the last processed offset tracking

Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #7518:
URL: https://github.com/apache/camel/pull/7518#issuecomment-1112376676

   :heavy_check_mark: Finished component verification: 0 component(s) test failed out of **1 component(s) tested**


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [camel] orpiske commented on pull request #7518: CAMEL-18024: rework the last processed offset tracking

Posted by GitBox <gi...@apache.org>.
orpiske commented on PR #7518:
URL: https://github.com/apache/camel/pull/7518#issuecomment-1133554526

   > > @orpiske the async commit process seems to be broken. The commit() function in DefaultKafkaManualAsyncCommit calls a forceCommit() function which is synchronous. And when fixing it using the commit() function (async with the manager), the test throw a ConcurrentModificationException(). We need to reintroduce a ConcurrentQueue storing commits which should be process by the main kafka consumer polling loop and thread.
   > 
   > Please, can you please open a ticket for this or send a PR with your proposed changes?
   > 
   > The important requirement is that the concurrent queue must be isolated to only the async-related code. It should not be part of the other code (ie.: the concerns of the async manual commit code should be separated) nor should be present in any way in the KafkaConsumer, KafkaFetchRecords or Kafka*Processor code. This is important so that we can continue cleaning up and reducing the maintenance effort for this component.
   
   Alternatively, on your scenario, can you also please test if changing the call from `forceCommit` to `recordOffset` leads to the correct behavior? 
   
   I believe the offset will eventually be [committed by the processor](https://github.com/apache/camel/blob/c2f6d79bc65d1db4e142bd082086c2c67edc78d8/components/camel-kafka/src/main/java/org/apache/camel/component/kafka/consumer/support/KafkaRecordProcessorFacade.java#L91) after successful processing.
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [camel] orpiske commented on pull request #7518: CAMEL-18024: rework the last processed offset tracking

Posted by GitBox <gi...@apache.org>.
orpiske commented on PR #7518:
URL: https://github.com/apache/camel/pull/7518#issuecomment-1128842917

   > 
   
   Hi, do you have a reproducer I can quickly transform into an integration test so I can take a look at it and make sure it works/won't break again? 
   
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [camel] orpiske commented on pull request #7518: CAMEL-18024: rework the last processed offset tracking

Posted by GitBox <gi...@apache.org>.
orpiske commented on PR #7518:
URL: https://github.com/apache/camel/pull/7518#issuecomment-1133553204

   And once again: thanks for the review and testing this part of the code. It's not a scenario that is widely used/tested, so I appreciate additional eyes on it. 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [camel] orpiske commented on pull request #7518: CAMEL-18024: rework the last processed offset tracking

Posted by GitBox <gi...@apache.org>.
orpiske commented on PR #7518:
URL: https://github.com/apache/camel/pull/7518#issuecomment-1133560158

   @ludovic-boutros I've got some time to look at this: https://github.com/apache/camel/pull/7664. Changing to recordOffset seems to do the trick. I'd kindly appreciate if you could give the fix a try on your scenario. 
   
   Thanks.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [camel] ludovic-boutros commented on pull request #7518: CAMEL-18024: rework the last processed offset tracking

Posted by GitBox <gi...@apache.org>.
ludovic-boutros commented on PR #7518:
URL: https://github.com/apache/camel/pull/7518#issuecomment-1128825463

   Hi @orpiske , I just checked this PR. The async ConcurrentQueue is mandatory if you want to be able to commit offsets in an async process like an aggregation with completion timeout. this is because the commit function must be called by the consumer thread. If not you will get a ConcurrentModification exception.
   The use case is for instance to index documents in a bulk manner using aggregate and commit once indexed.
    


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [camel] github-actions[bot] commented on pull request #7518: CAMEL-18024: rework the last processed offset tracking

Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #7518:
URL: https://github.com/apache/camel/pull/7518#issuecomment-1112282466

   :x: Finished component verification: **1 component(s) test failed** out of 4 component(s) tested


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [camel] github-actions[bot] commented on pull request #7518: CAMEL-18024: rework the last processed offset tracking

Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #7518:
URL: https://github.com/apache/camel/pull/7518#issuecomment-1112299381

   :x: Finished component verification: **1 component(s) test failed** out of 4 component(s) tested


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [camel] orpiske merged pull request #7518: CAMEL-18024: rework the last processed offset tracking

Posted by GitBox <gi...@apache.org>.
orpiske merged PR #7518:
URL: https://github.com/apache/camel/pull/7518


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org