Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CAMEL-18019: ensure some fields in the Kafka record fetcher are safe for concurrent read #7503

Merged
merged 1 commit into from Apr 28, 2022

Conversation

orpiske
Copy link
Contributor

@orpiske orpiske commented Apr 27, 2022

No description provided.

@github-actions
Copy link
Contributor

⚠️ This PR changes Camel components and will be tested automatically.

@github-actions
Copy link
Contributor

✔️ Finished component verification: 0 component(s) test failed out of 1 component(s) tested

@essobedo
Copy link
Contributor

What about clientId and lastError? They seem to be shared also

@essobedo
Copy link
Contributor

I see also a potential bug here. Indeed the pollExceptionStrategy is set in the constructor using the value of the field consumer that is always null at this stage

@essobedo
Copy link
Contributor

One more potential issue, the map lastProcessedOffset seems to be read and write by different threads, It looks like it should be a ConcurrentHashMap

@orpiske
Copy link
Contributor Author

orpiske commented Apr 27, 2022

I see also a potential bug here. Indeed the pollExceptionStrategy is set in the constructor using the value of the field consumer that is always null at this stage

Please open a separate ticket for this one.

@orpiske
Copy link
Contributor Author

orpiske commented Apr 27, 2022

I think we should probably abstract the task health state that is exposed to the HealthCheck, so it's easier to know what is being accessed elsewhere.

@essobedo
Copy link
Contributor

I see also a potential bug here. Indeed the pollExceptionStrategy is set in the constructor using the value of the field consumer that is always null at this stage

Please open a separate ticket for this one.

https://issues.apache.org/jira/browse/CAMEL-18020

@orpiske
Copy link
Contributor Author

orpiske commented Apr 27, 2022

One more potential issue, the map lastProcessedOffset seems to be read and write by different threads, It looks like it should be a ConcurrentHashMap

I think we could try to remove this altogether and move its functionality to the commit manager. This is a remainder for the earlier versions of this component (pre 3.7), so I believe what it is trying to do overlaps with what we have there.

@orpiske
Copy link
Contributor Author

orpiske commented Apr 27, 2022

We can also probably remove the reconnect variable at this point (or leave in the appropriate PollExceptionStrategy). Anyway, let's do it in baby steps so it's easier to bisect if we have problems.

@orpiske
Copy link
Contributor Author

orpiske commented Apr 28, 2022

One more potential issue, the map lastProcessedOffset seems to be read and write by different threads, It looks like it should be a ConcurrentHashMap

I logged this one here: https://issues.apache.org/jira/browse/CAMEL-18020 (as explained, I think this one should be reworked and moved to a more appropriate location within the code).

@github-actions
Copy link
Contributor

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

@github-actions
Copy link
Contributor

✔️ Finished component verification: 0 component(s) test failed out of 1 component(s) tested

@orpiske
Copy link
Contributor Author

orpiske commented Apr 28, 2022

I'm going to merge this first bit of changes. Please keep the reviews, comments, tickets going ...

@orpiske orpiske merged commit d1f919a into apache:main Apr 28, 2022
@orpiske orpiske deleted the camel-18019 branch August 16, 2022 11:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants