Repository navigation
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
On every read that returns block locations (
GetKeyInfo,lookupKey,getFileStatus,listStatus), OM sorts each block's datanodes by distance to the client:KeyManagerImpl.sortDatanodes→NetworkTopologyImpl.sortByDistanceCost→getDistanceCostonce per node. EachgetDistanceCostcall takes the read lock ofnetlock, a single global fairReentrantReadWriteLock. For non-datanode clients,getOtherNodealso takes it once per request to look up the client's rack.With many concurrent readers, every read-lock acquisition CASes one shared state word and looks up a per-thread hold count in a
ThreadLocal. With 100+ handlers this turns into CAS retries and longThreadLocalprobes, OM CPU climbs while throughput falls, and slower calls keep more handlers in flight.Observed in production (2.2.1, 3-node OM HA, ~32 datanodes, 168-core OM host):
GetKeyInfotakes ~6 ms against 1.5–2.5 ms at baseline, at a lower call rate.om_performance_metrics_get_key_info_sort_datanodes_latency_ns_avg_time: 0.01 ms quiet, 4.2 ms at the burst peak, ~70% of the call.ThreadLocal$ThreadLocalMap.getEntryAfterMiss(9% at baseline). Of those, 85% come fromReadLock.lockingetDistanceCost←sortByDistanceCost, and 15% fromgetNode←KeyManagerImpl.getOtherNode.HDFS solved this long ago:
NetworkTopologyhas the same fairnetlock, butsortByDistancecomputes weights withgetWeight, a lock-free walk of parent pointers that compares nodes by path (NodeBase.equals). This PR applies the same approach.Changes
NetworkTopologyImpl: lock-free distance walk for sorting.getDistanceCostWithoutLock, whichsortByDistanceCostuses.getDistanceCostkeeps the lock for its other callers and delegates to the same method, so the two cannot diverge.InnerNodeImpl.equals. That method compares whole child maps when the two nodes are different instances; without the lock it could iterate a map that SCM is modifying.getCost()sums,Integer.MAX_VALUEfor nodes outside the tree). HDFS's hop count is not used, because Ozone schemas support per-level costs.KeyManagerImpl: cache resolved non-datanode clients.getOtherNodecaches the clientNodeper client host. This removes the per-requestgetNodelock, the mapping lookup and aNodeImplallocation.ScmTopologyClientreplaces the instance on refresh, and the cache is replaced with it, so a cached node always points at the current tree.ozone.om.network.topology.refresh.duration(default 1h).Edge cases
AtomicReference, and never modified, so the lock protected nothing there. Results are identical.nullafter the ancestor check, the walk returnsMAX_VALUEinstead of a partial cost, so a node being removed never sorts first.ScriptBasedMappingcaches successful lookups forever, so nothing changes for it. A mapping configured directly, such asStaticMappingor a custom class, is now consulted at most once per refresh interval per client.getNodelookup per request.MAX_VALUE(cheaply now). That is fixed separately in HDDS-17555.What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-17553
How was this patch tested?
New tests:
TestNetworkTopologyImpl:testSortByDistanceCostDoesNotTakeLockPerNode: sorting never calls the lockedgetDistanceCost. It fails on the old code.testSortByDistanceCostWhileTopologyChanges: four threads sort while a fifth removes and re-adds half the nodes, on all five schema topologies. Every result contains all nodes, and the nodes that aren't changing stay ordered by cost.testDistanceCostIsMaxWhenNodeIsRemovedDuringWalk: a node that loses its parent after the ancestor check costsMAX_VALUE. The old code gave it 1, below a same-rack node's 2.TestOMSortDatanodes:cachesResolvedClientPerClusterMap: same node for the same map, a new one for a new map instance.cachesClientOnRackWithoutDatanodes: the "no rack in this map" result is cached per map instance.cachedClientExpiresAfterTopologyRefreshInterval: an entry expires after the refresh interval.