Arvados

Changes at arvados|dea8c169f9d38e5a7c518514450c0955648d58bd branch 21204-stable-log-sort
Tests developer-run-tests-services-workbench2: #81

• All agreed upon points are implemented / addressed.
  • Changed sorting to preserve original ordering of lines of the same type
  • Lines with different types but matching timestamps are sorted by type, the result is that lines of matching timestamp will be grouped by type one after another, with lines within a type preserving original order
  • Added more tests for preserving original ordering of sorted lines within a single type while correctly ordering/grouping matching timestamp lines of differing types
• Anything not implemented (discovered or discussed during work) has a follow-up story.
  • none
• Code is tested and passing, both automated and manual, what manual testing was done is described
  • done, added test cases to ensure correct sorting brhavior
• Documentation has been updated.
  • n/a
• Behaves appropriately at the intended scale (describe intended scale).
  • Minimal performance impact expected, since only the sort comparator function was tweaked and a reduce/concat replaced with a map/reduce to provide the necessary information for the sort comparator
• Considered backwards and forwards compatibility issues between client and server.
  • n/a
• Follows our coding standards and GUI style guidelines.
  • n/a

I cherry picked the 2 commits to the arvados-workbench2 repo at arvados-workbench2|b774da617da87999f68863b90542dd41d9ba0bd3 21204-stable-log-sort

Stephen Smith wrote in #note-4:

Changes at arvados|dea8c169f9d38e5a7c518514450c0955648d58bd branch 21204-stable-log-sort
Tests developer-run-tests-services-workbench2: #81

• All agreed upon points are implemented / addressed.
  • Changed sorting to preserve original ordering of lines of the same type
  • Lines with different types but matching timestamps are sorted by type, the result is that lines of matching timestamp will be grouped by type one after another, with lines within a type preserving original order
  • Added more tests for preserving original ordering of sorted lines within a single type while correctly ordering/grouping matching timestamp lines of differing types
• Anything not implemented (discovered or discussed during work) has a follow-up story.
  • none
• Code is tested and passing, both automated and manual, what manual testing was done is described
  • done, added test cases to ensure correct sorting brhavior
• Documentation has been updated.
  • n/a
• Behaves appropriately at the intended scale (describe intended scale).
  • Minimal performance impact expected, since only the sort comparator function was tweaked and a reduce/concat replaced with a map/reduce to provide the necessary information for the sort comparator
• Considered backwards and forwards compatibility issues between client and server.
  • n/a
• Follows our coding standards and GUI style guidelines.
  • n/a

So, this works and I'm fine with it, LGTM.

But I suspect if you moved timestampMatch into sortableLineSortFunc you could eliminate the map/reduce step and also avoid parsing lines in the common case where the lines are the same log type.

In fact you probably don't need to parse the timestamp at all, unless you have a line that doesn't have a leading timestamp you can directly compare two lines with leading timestamps, the timestamp text is fixed-width and lexically sortable already.