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

Fix concurrent map read/write in partition management #454

Open
wants to merge 1 commit into
base: v7
Choose a base branch
from

Conversation

ravisastryk
Copy link

Problem
The current implementation of partitionMap is not thread-safe, leading to potential race conditions during concurrent read and write operations. This issue affects various parts of the codebase, including cluster management, node operations, and partition parsing.

Issue References
#446
#399

Solution
Implement a thread-safe partitionMap using sync.Map and update all related functions to use the new thread-safe methods. This change ensures safe concurrent access to partition data across multiple goroutines.

Technical Notes
partitionMap is a thread-safe map that stores partition information for different namespaces. It uses a sync.Map internally to provide concurrent read/write access without explicit locking. The keys are namespace names (strings), and the values are pointers to Partitions structs. This structure allows for efficient, concurrent operations on partition data across multiple goroutines.

Usage:
 - Use get(key) to retrieve partition data for a namespace
 - Use set(key, value) to update or add partition data for a namespace
 - Use iterate(func) to iterate over all namespace-partition pairs safely
 - Use delete(key) to remove partition data for a namespace
 - Use len() to get the number of namespaces in the map

Changes
partitions.go

  • Redefined partitionMap to wrap a sync.Map
  • Implemented thread-safe methods: get, set, delete, len, and iterate
  • Updated clone and cleanup methods to work with the new structure

partition.go

  • Modified PartitionForWrite and PartitionForRead to use new get method of partitionMap

partition_parser.go

  • Updated partitionParser struct to use a pointer to partitionMap
  • Modified parsing logic to use new thread-safe methods of partitionMap

node.go

  • Updated refreshPartitions method to work with the new partitionMap structure

cluster.go

  • Changed partitionWriteMap in Cluster struct to use a pointer to partitionMap
  • Updated setPartitions, getPartitions, and findNodeInPartitionMap methods
  • Modified NewCluster function to initialize partitionWriteMap correctly
  • Updated tend method to use thread-safe operations on partitionMap

@ravisastryk
Copy link
Author

ravisastryk commented Oct 29, 2024

@khaf
Can you please review this as soon as possible? It seems that many people are awaiting this fix. Let's aim to make it available to all.

cc @Gaudeamus

@khaf
Copy link
Collaborator

khaf commented Oct 30, 2024

@ravisastryk The partitionMap structure is a lockless construct. We have specifically avoided synchronizing the map. Using sync.Map has major performance consequences. As such, this PR is not workable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants