goiri commented on code in PR #6096:
URL: https://github.com/apache/hadoop/pull/6096#discussion_r1329005745
##########
hadoop-hdfs-project/hadoop-hdfs-rbf/src/test/java/org/apache/hadoop/hdfs/server/federation/store/TestStateStoreMembershipState.java:
##########
@@ -223,6 +223,57 @@ public void testRegistrationMajorityQuorum()
assertEquals(quorumEntry.getRouterId(), ROUTERS[3]);
}
+ /**
+ * Fix getRepresentativeQuorum when records have same date modified time.
+ */
+ @Test
+ public void testRegistrationMajorityQuorumEqDateModified()
+ throws InterruptedException, IOException {
+
+ // Populate the state store with a set of non-matching elements
+ // 1) ns0:nn0 - Standby (newest)
+ // 2) ns0:nn0 - Active
+ // 3) ns0:nn0 - Active
+ // 4) ns0:nn0 - Active
+ // (2), (3), (4) have the same date modified time
+ // Verify the selected entry is the newest majority opinion (4)
+ String ns = "ns0";
+ String nn = "nn0";
+
+ long dateModified = Time.now();
+ // Active - oldest
+ MembershipState report = createRegistration(
+ ns, nn, ROUTERS[1], FederationNamenodeServiceState.ACTIVE);
Review Comment:
Tweak the spacing to fit checkstyle.
##########
hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/store/impl/MembershipStoreImpl.java:
##########
@@ -279,30 +279,30 @@ private MembershipState getRepresentativeQuorum(
Collection<MembershipState> records) {
// Collate objects by field value: field value -> order set of records
- Map<FederationNamenodeServiceState, TreeSet<MembershipState>> occurenceMap
=
+ Map<FederationNamenodeServiceState, Set<MembershipState>> occurenceMap =
new HashMap<>();
for (MembershipState record : records) {
FederationNamenodeServiceState state = record.getState();
- TreeSet<MembershipState> matchingSet = occurenceMap.get(state);
+ Set<MembershipState> matchingSet = occurenceMap.get(state);
if (matchingSet == null) {
- // TreeSet orders elements by descending date via comparators
- matchingSet = new TreeSet<>();
+ matchingSet = new HashSet<>();
occurenceMap.put(state, matchingSet);
}
matchingSet.add(record);
}
// Select largest group
- TreeSet<MembershipState> largestSet = new TreeSet<>();
- for (TreeSet<MembershipState> matchingSet : occurenceMap.values()) {
+ Set<MembershipState> largestSet = new HashSet<>();
Review Comment:
> This bug is because the above record is put into the Treeset in the method
getRepresentativeQuorum. Since record1,2,3 have the same dateModified, there
will only be one record in the final treeset of this method, so this method
thinks that this nn is standby, because record4 newer
OK, so the issue is with the comparator and seems like going to HashSet, we
fix the comparisson.
Shouldn't we be explicit about this instead of relying on TreeSet or HashSet
using different comparators?
My proposal would be to fix that part.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
