Uploaded image for project: 'Core Server'
  1. Core Server
  2. SERVER-58309

Clean up benchRun()'s "safe" option, and related options

    • Type: Icon: Task Task
    • Resolution: Fixed
    • Priority: Icon: Major - P3 Major - P3
    • 5.1.0-rc0
    • Affects Version/s: None
    • Component/s: None
    • Labels:
      None
    • Fully Compatible
    • Query Execution 2021-07-26, QE 2021-08-09

      As far as I understand it, the "safe" option supported by benchRun() used to be a way for a caller to opt into using the getLastError() command in order to check that write operations have succeeded as expected. It also appears to have enabled some logging about the results of operations run by benchRun(), but only if "hideResults" is false or "showResult" is true. (By default, it looks like "hideResults" is true and "showResult" is false, meaning that this logging will be suppressed.)

      We are preparing to remove support for the getLastError command, and in a previous work item we cleaned up benchRun() to require write commands and never use getLastError. However, we retained some getLastError-related code which is enabled by the "safe" option. For example:

      https://github.com/mongodb/mongo/blob/e1f9a3922d20886691e4c22c4fa63ba81a89b3d7/src/mongo/shell/bench.cpp#L1229-L1238

      I don't think this code makes sense anymore without getLastError. It looks for a field in the response document called "err", which is a format only used by the getLastError command. We have a few options to fix this:

      • Simply delete "safe" mode, as well as the accompanying options "throwGLE", "hideResults", and "showResult". This is assuming that we don't have any callers of benchRun() which use "safe" mode or the other options named above.
      • Re-implement "safe" mode so that it checks the BSONObj response received after running the write command. If there is an error, we could either log something about the error or throw an exception, depending on the value of some other parameter (e.g. "throwOnError").

      Since we are trying to replace benchRun() with other perf testing tools (e.g. Genny) we probably should do something easy here. That said, I still think the code as written is worth fixing since it is simply incorrect; it makes getLastError-related checks in a world without getLastError.

            Assignee:
            jennifer.peshansky@mongodb.com Jennifer Peshansky (Inactive)
            Reporter:
            david.storch@mongodb.com David Storch
            Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

              Created:
              Updated:
              Resolved: