Opened 10 months ago

Closed 8 months ago

#25085 closed enhancement (fixed)

Make order of sorted results deterministic

Reported by: iwakeh Owned by: iwakeh
Priority: Medium Milestone: Onionoo 1.11.0
Component: Metrics/Onionoo Version:
Severity: Normal Keywords:
Cc: metrics-team Actual Points:
Parent ID: #25002 Points:
Reviewer: karsten Sponsor:

Description

A non-deterministic element in Onionoo docs is the order of entries. When there is a tie in the used order parameter order is arbitrary and will differ. A simple solution could be to always sort by fingerprint in case of a tie.

See this patch.

Open steps:

  • rebase to master,
  • add a change log entry, and
  • provide a metrics-web patch for updating the ​specification

Child Tickets

Change History (14)

comment:1 Changed 10 months ago by iwakeh

Owner: changed from metrics-team to iwakeh
Status: newaccepted

comment:2 Changed 9 months ago by iwakeh

Please review two commits on the rebased branch including the changelog entry.

(Protocol update follows.)

comment:3 Changed 9 months ago by iwakeh

Status: acceptedneeds_review

comment:4 Changed 9 months ago by iwakeh

Please also review (and possibly improve) the specification changes.

comment:5 Changed 9 months ago by karsten

Cc: metrics-team added
Reviewer: karsten

I'll review this. (I didn't see the more recent updates, because metrics-team was not copied. Fixing that.)

comment:6 Changed 9 months ago by karsten

Status: needs_reviewneeds_revision

How about we sort all responses by fingerprint, not just when an order parameter is given? We could still use the fingerprint to break ties, but we'd also use it when no order parameter is given. Does that make sense? If so, want to prepare a modified patch?

comment:7 Changed 9 months ago by iwakeh

I thought about that, too, but am kind of vary to introduce the change. After all, it's been that way all along. On the other hand, I cannot think of a client side use case for the non-deterministic order.
Maybe, introduce another parameter for fingerprint sorting? But, that's more work.

Unless, you raise any other concern here, I'll prepare the changed patch.

Where there any other reasons for setting to needs_revision?

comment:8 in reply to:  7 Changed 9 months ago by karsten

Replying to iwakeh:

I thought about that, too, but am kind of vary to introduce the change. After all, it's been that way all along.

It's true that it has been like this all the time. It just feels like an improvement to make all responses deterministic, not just some.

On the other hand, I cannot think of a client side use case for the non-deterministic order.

No, it's nothing that clients can rely on.

Maybe, introduce another parameter for fingerprint sorting? But, that's more work.

That's some more work, yes. But what would be the goal? Have a default value for the order parameter? But what if clients ask for results ordered by "consensus_weight" without passing "fingerprint" as second field to order by? Then we'd be in the same situation as we're in now. I think fingerprint is really a special case here. It's like the primary key in a database sense.

Unless, you raise any other concern here, I'll prepare the changed patch.

No other concerns from me.

Where there any other reasons for setting to needs_revision?

Just this.

Thanks!

comment:9 Changed 8 months ago by karsten

Milestone: Onionoo 1.11.0

Optimistically assigning to the next milestone.

comment:10 Changed 8 months ago by iwakeh

Status: needs_revisionaccepted

comment:11 Changed 8 months ago by iwakeh

Status: acceptedneeds_review

Please find another commit (fixup) on the now correctly named patch branch for review.

The protocol (see patch in comment:4) doesn't need to be changed as the sorting is not part of the protocol and we don't want to guarantee any particular order besides the existing sort options.

comment:12 in reply to:  11 ; Changed 8 months ago by karsten

Replying to iwakeh:

Please find another commit (fixup) on the now correctly named patch branch for review.

Looks good. Here's a slightly tweaked change log entry. Please take a look.

The protocol (see patch in comment:4) doesn't need to be changed as the sorting is not part of the protocol and we don't want to guarantee any particular order besides the existing sort options.

Makes sense.

comment:13 in reply to:  12 Changed 8 months ago by iwakeh

Status: needs_reviewmerge_ready

Replying to karsten:

Replying to iwakeh:

Please find another commit (fixup) on the now correctly named patch branch for review.

Looks good. Here's a slightly tweaked change log entry. Please take a look.

All fine.

comment:14 Changed 8 months ago by karsten

Resolution: fixed
Status: merge_readyclosed

Squashed and merged! I think that's all. Closing. Thanks!

Note: See TracTickets for help on using tickets.