Skip to content

CAMEL-19295 - Concurrency issues with dynamicMap in AbstractDynamicRegistry - #9972

Merged
davsclaus merged 2 commits into
apache:mainfrom
rhuan080:CAMEL-19295
May 19, 2023
Merged

CAMEL-19295 - Concurrency issues with dynamicMap in AbstractDynamicRegistry#9972
davsclaus merged 2 commits into
apache:mainfrom
rhuan080:CAMEL-19295

Conversation

@rhuan080

@rhuan080 rhuan080 commented May 1, 2023

Copy link
Copy Markdown
Contributor

Description

Target

  • I checked that the commit is targeting the correct branch (note that Camel 3 uses camel-3.x, whereas Camel 4 uses the main branch)

Tracking

  • If this is a large change, bug fix, or code improvement, I checked there is a JIRA issue filed for the change (usually before you start working on it).

Apache Camel coding standards and style

  • I checked that each commit in the pull request has a meaningful subject line and body.
  • I formatted the code using mvn -Pformat,fastinstall install && mvn -Psourcecheck

rhuan080 added 2 commits May 1, 2023 20:00
Signed-off-by: Rhuan Rocha <rhuan080@gmail.com>
Signed-off-by: Rhuan Rocha <rhuan080@gmail.com>
@github-actions

github-actions Bot commented May 1, 2023

Copy link
Copy Markdown
Contributor

🌟 Thank you for your contribution to the Apache Camel project! 🌟

⚠️ Please note that the changes on this PR may be tested automatically.

If necessary Apache Camel Committers may access logs and test results in the job summaries!

@rhuan080

rhuan080 commented May 1, 2023

Copy link
Copy Markdown
Contributor Author

Hi, I sent this PR and I have seen the SimpleLRUCache [1] is not thread-safe, and we don`t have a ConcurrentLinkedHashMap or something like that. Thus, I think we need to review this class to turn it thread-safe. Maybe an improvement in another issue?

[1] -

private class SimpleLRUCache<K, V> extends LinkedHashMap<K, V> {

@rhuan080 rhuan080 changed the title Camel 19295 Camel 19295 - Concurrency issues with dynamicMap in AbstractDynamicRegistry May 1, 2023
@rhuan080 rhuan080 changed the title Camel 19295 - Concurrency issues with dynamicMap in AbstractDynamicRegistry Camel-19295 - Concurrency issues with dynamicMap in AbstractDynamicRegistry May 1, 2023
@rhuan080 rhuan080 changed the title Camel-19295 - Concurrency issues with dynamicMap in AbstractDynamicRegistry CAMEL-19295 - Concurrency issues with dynamicMap in AbstractDynamicRegistry May 1, 2023
@github-actions

github-actions Bot commented May 2, 2023

Copy link
Copy Markdown
Contributor

🚫 There are (likely) no components to be tested in this PR

@davsclaus

Copy link
Copy Markdown
Contributor

Yes we should look at SimpleLRUCache before 4.0, can you create a JIRA ticket

@davsclaus
davsclaus merged commit fab07a0 into apache:main May 19, 2023
@orpiske

orpiske commented May 19, 2023

Copy link
Copy Markdown
Contributor

Yes we should look at SimpleLRUCache before 4.0, can you create a JIRA ticket

I'm wondering if it would be possible to unify all these caches and pools that we have. For instance, we have the ServicePool which has overlapping features with the LRUCache.

davsclaus added a commit that referenced this pull request May 19, 2023
davsclaus added a commit that referenced this pull request May 19, 2023
@davsclaus

Copy link
Copy Markdown
Contributor

Okay had to revert as this causes many other tests to fail
#10155

@rhuan080 thanks for the PR, but when changing in core, then its best to run the tests in camel-core to ensure its not affected other parts

@davsclaus

Copy link
Copy Markdown
Contributor

Yes we should look at SimpleLRUCache before 4.0, can you create a JIRA ticket

I'm wondering if it would be possible to unify all these caches and pools that we have. For instance, we have the ServicePool which has overlapping features with the LRUCache.

Yeah but ServicePool is a bit complex and special for camel's producer/consumer caches.
Not really for end users. But if it can be made simpler then that would be good - its a bit tricky to maintain.

@davsclaus

Copy link
Copy Markdown
Contributor

We may just drop SimpleLRUCache and use camel-caffeine-lrucache by default, and maybe move it into root/core folder. This cache is concurrent and LRU based. However it has a slight overhead on startup, that Simple does not have.

@otavioprado something you may have fun with performance testing in your lab.

@davsclaus

Copy link
Copy Markdown
Contributor

I did a quick experiment with fully sync LRUCache and this unit test
#10157

It all works, but since its fully sync it may have an overhead.
To use caffeine, you can even today, just drop camel-caffeine-lrucache in the classpath and Camel uses this instead.
So end users can choose.

And we could make the simple just synchronized (but maybe slower).

@otavioprado you are welcome to test in lab

@orpiske

orpiske commented May 19, 2023

Copy link
Copy Markdown
Contributor

@davsclaus that's a good one to test. I'll write a test for it and will do it next week. Let's see how it goes!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants