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

Make EventCounter thread safe #4959

Merged
merged 1 commit into from
Oct 8, 2024

Conversation

DomGarguilo
Copy link
Member

Fixes #4956

The stack track included in the ticket indicates that a ConcurrentmodificationException occurred in Monitor.EventCounter:

java.util.ConcurrentmodificationException
at java.util.HashMap$HashIterator.nextNode(HashMap.java:1511)
at java.util.HashMap$HashIterator.next(HashMap.java:1544)
at java.util.HashMap$HashIterator.next(HashMap.java:1542)
at org.apache.accumulo.monitor.Monitor$EventCounter.calculateRate(monitor.java:23)
at org.apache.accumulo.monitor.Monitor.getLookupRate(Monitor.java:977)
at org.apache.accumulo.monitor.rest.tservers.TabletServerInformation.updateTabletServerInfo(TabletServerinformation.java:162)
at org.apache.accumulo.monitor.rest.tservers.TabletServerInformation.(TabletServerInformation:101)
at org.apache.accumulo.monitor.rest.tservers.TabletServer.(TabletServer.java:43)
at org.apache.accumulo.monitor.rest.tservers.TabletServerResource.getTserverSummary(TabletServerResource.java:88)

This PR adds synchronization to several methods in the EventCounter class to avoid this from happening again.

Another approach that would probably work would be to make the 3 collections in this class thread safe by making the HashMaps into ConcurrentHashMaps and the HashSet into a Collections.synchronizedSet(new HashSet<>()). That would function just fine in the current code as far as I can tell but if we would ever need to iterate over the synchronizedSet, we would need to use a synchronize block anyways so i figured just making the methods themselves synchronized would future proof and fix the current issue too.

@DomGarguilo DomGarguilo added the bug This issue has been verified to be a bug. label Oct 8, 2024
@DomGarguilo DomGarguilo added this to the 2.1.4 milestone Oct 8, 2024
@DomGarguilo DomGarguilo self-assigned this Oct 8, 2024
@DomGarguilo DomGarguilo merged commit e733bd3 into apache:2.1 Oct 8, 2024
8 checks passed
@DomGarguilo DomGarguilo deleted the makeEventCounterThreadSafe branch October 8, 2024 21:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue has been verified to be a bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Monitor threw ConcurrentModificationException when loading Tablet Server page
2 participants