envoyproxy / nighthawk

L7 (HTTP/HTTPS/HTTP2/HTTP3) performance characterization tool
Apache License 2.0
361 stars 81 forks source link

Consider consolidating IsConverged and IsDoomed in Adaptive Load StepController #466

Open eric846 opened 4 years ago

eric846 commented 4 years ago

Both are const methods that can change value from the same events.

oschaaf commented 4 years ago

I was looking at https://github.com/envoyproxy/nighthawk/pull/492, another thought related to this topic So please hear me out ... currently on master StepController looks like:

  virtual absl::StatusOr<nighthawk::client::CommandLineOptions> GetCurrentCommandLineOptions() const PURE;
  virtual bool IsConverged() const PURE;
  virtual bool IsDoomed(std::string& doom_reason) const PURE;
  virtual void UpdateAndRecompute(const nighthawk::adaptive_load::BenchmarkResult& result) PURE;

I am now wondering, what if StepController would condense these 4 methods into a single one. Thinking it through, I end up with something like:

  //  Return value imposes three possible transitions for consuming code:  
  //    - StatusOr indicated success AND the inner unique_ptr == nullptr: converged / terminate succesfully
  //    - StatusOr indicated success AND  the inner unique_ptr != nullptr: more work
  //    - StatusOr indicated failure: doomed / terminate with an error
  // result parameter can equate to nullptr, implying the first call. This might be ASSERTED on to avoid mistakes in consuming code.
  virtual absl::StatusOr<std::unique_ptr<nighthawk::client::CommandLineOptions>> 
       NextCommandLine(const std::unique_ptr<const nighthawk::adaptive_load::BenchmarkResult>& result) const PURE;

This thought wouldn't make sense if it is valid/desirable that consuming (prod) code is able to call UpdateAndRecompute multiple times without interleaving it with GetCurrentCommandLineOptions() as it stands on master. But if that's not the case, then doing this could be considered api abuse, and collapsing the interface to a single method might rule that out?