[SERVER-17410] listCommands returns unordered list Created: 27/Feb/15  Updated: 05/Feb/16  Resolved: 07/Jul/15

Status: Closed
Project: Core Server
Component/s: Usability
Affects Version/s: None
Fix Version/s: 3.1.6

Type: Improvement Priority: Trivial - P5
Reporter: Jonathan Abrahams Assignee: J Rassi
Resolution: Done Votes: 0
Labels: 28qa, neweng
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
Related
is related to SERVER-19308 Basic test coverage for listCommands ... Closed
Backwards Compatibility: Fully Compatible
Sprint: Quint Iteration 6
Participants:

 Description   

The command db.adminCommand({listCommands:1}) returns a JSON result with a list of commands supported by the server. In 2.6 this was ordered by command name (sorted). In 3.0 it has no order.



 Comments   
Comment by Githook User [ 07/Jul/15 ]

Author:

{u'username': u'upendrag', u'name': u'Upendra Gowda', u'email': u'upendrag.gowda@gmail.com'}

Message: SERVER-17410: listCommands returns unordered list

Closes #990

Signed-off-by: Jason Rassi <rassi@10gen.com>
Branch: master
https://github.com/mongodb/mongo/commit/de11b1cae53290aa8100185e90f7894d0adc80f8

Comment by Ramon Fernandez Marina [ 02/Jul/15 ]

upendrag, #990 is being tested as we speak; if all testing passes then we'll merge your code. So far it's looking good, so feel free to start other tasks, we'll let you know if any further tweaking is needed.

Regards,
Ramón.

Comment by Upendra Gowda [ 02/Jul/15 ]

Hi ramon.fernandez,

Just a quick question. Now that my patch has been 'LGTM'ed, can I assume that the work is done and start looking into new tasks?

Thanks,
Upendra

Comment by Ramon Fernandez Marina [ 30/Jun/15 ]

Thanks upendrag, sending your new PR for review.

Comment by Upendra Gowda [ 30/Jun/15 ]

Please find the new pull request at https://github.com/mongodb/mongo/pull/990

Comment by Upendra Gowda [ 30/Jun/15 ]

Yes ramon.fernandez. I think you are right. This happened due to recent 'clang' formatting. I'll close the existing pull request and open a new one based on the most recent commit.

Comment by Ramon Fernandez Marina [ 30/Jun/15 ]

Thanks for updating the pull request upendrag. Unfortunately something went wrong during this process, as your pull request now contains over 250 commits and changes to over 2400 files! I can no longer see all the changes you propose without using the github API, which makes it very hard for anyone to review your work.

I'll see if I can extract your proposed changes from this pull request, but it may be simpler (and clearer) to close it and open a new one containing only your changes for this ticket. What do you think?

Comment by Upendra Gowda [ 29/Jun/15 ]

Hi rassi@10gen.com,

I have updated the pull request. Please review it and let me know if there are any comments.

Thanks,
Upendra

Comment by Ramon Fernandez Marina [ 19/Jun/15 ]

Thanks for the pull request upendrag! Will find a reviewer to take a look at your code and get back to you soon.

Comment by Upendra Gowda [ 18/Jun/15 ]

I have attached a link to the pull request in this ticket. Please review and let me know your comments.

Thanks,
Upendra

Comment by J Rassi [ 09/Jun/15 ]

upendrag: I'd go with whichever of #1 or #2 produces more readable code. "listCommands" is an infrequently-used administrative command that returns <50k of data and has no strict latency requirements, so the difference in speed or memory usage between those two doesn't seem particularly significant in this case.

That said, a C++ suggestion for #2: you should generally prefer std::vector<> to std::list<> when you don't need fast insertion or removal at an arbitrary point in the list.

Looking forward to your pull request!

~ Jason Rassi

Comment by Upendra Gowda [ 09/Jun/15 ]

Thanks for the feedback ramon.fernandez.

Since the commands are stored as UnorderedFastKeyTable (a Hashtable with linear probing), I am not sure if it is possible to sort them directly. The 2 other approaches that I could think of are

Approach 1: Construct a map<commandName, CommandObj> from commands table and iterate through this map to build the result BSON obj. This is fast but there is a memory overhead.

Approach 2: Construct a list<commandName> from commands table and iterate through this list while retrieving Command Objects from commands table using the commandName to build the result BSON obj. This approach is not as fast as "approach 1" but uses less memory.

Comment by Ramon Fernandez Marina [ 08/Jun/15 ]

Thanks for following up in the ticket upendrag. Before we move forward I'd strongly recommend you take a look at our contributors page, specially the getting started section which talks about the contributor's agreement, setting up a fork, etc.

The ticket can't be assigned to you, so all the work happens via pull requests in github. Note that the preferred approach here would be to sort the commands before returning them, so I think you should consider that option as well – but if you think other options are better you're welcome to argue your case here.

Please check out the links above and let us know if you have any further questions.

Thanks,
Ramón.

Comment by Upendra Gowda [ 08/Jun/15 ]

Hi Ramon/Jonathan,

I believe the issue was caused by the commit https://github.com/mongodb/mongo/commit/edb2a9a1a5d13c3d11381a4f4370584d00bad883.

The data structure holding the "Command::_commands" object in "src/mongo/db/dbcommands_generic.cpp" was changed from map<string,Command*> to CommandMap which is of type UnorderedFastKeyTable<...>. Before this change, the result bson object was sorted by command names because it is constructed by iterating through "_commands" in "ListCommandsCmd::run()" and "_commands" was naturally ordered by map's key (command name).

A straightforward fix would be to have a temporary sorted container for commands and iterating through this temporary container to construct the result bson object. There is a trade off with performance though.

I think this an ideal task for beginner. Shall I go ahead and assign the ticket to myself and start working on alternative solutions?

Thanks,
Upendra

Generated at Thu Feb 08 03:44:20 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.