Skip to content

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