HIVE-28874 Incorrect comparison in LlapFixedRegistryImpl #5742
Conversation
|
| @@ -257,7 +257,7 @@ public List<LlapServiceInstance> getAllInstancesOrdered(boolean consistentIndexe | |||
| Collections.sort(list, new Comparator<LlapServiceInstance>() { | |||
| @Override | |||
| public int compare(LlapServiceInstance o1, LlapServiceInstance o2) { | |||
| return o2.getWorkerIdentity().compareTo(o2.getWorkerIdentity()); | |||
| return o1.getWorkerIdentity().compareTo(o2.getWorkerIdentity()); | |||
There was a problem hiding this comment.
Doesn't this change the sorting order? I think now you will see it in ascending order when compared to descending order.
There was a problem hiding this comment.
Previously there was no order (and inconsistency instead) as o2 was compared to o2 and they are always equal.
Ascending Order (o1.compareTo(o2))
Descending Order (o2.compareTo(o1))
Please let me know if descending order is expected
There was a problem hiding this comment.
Do we really need this sorting then since it was working without it.
cc @abstractdog Do you think it makes sense to use sorting here?
There was a problem hiding this comment.
Considering that the comparison was incorrect and FixedServiceInstanceSet failed to comply with any sorting requirements (unlike DynamicServiceInstanceSet, which uses a TreeMap to maintain order), it doesn't really matter whether we fix the sorting or remove it entirely.
However, since the interface implies that some ordering is expected, it's reasonable to fix it now using a simple ascending order, which aligns better with intuitive expectations and brings consistency to the implementation.
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
|
dear maintainers, please review |
left a comment above, this is fine to be merged, can you please rebase your patch on master to get a green build? |
|
|
@abstractdog please merge |
No description provided.