runtime: add getAll accessor to Snapshot to access underlying Entry map #2311

rodaine · 2018-01-04T02:16:29Z

This patch adds a getAll accessor to the underlying entries map in the Runtime::Snapshot. This will be used in the forthcoming /runtime admin endpoint (#514).

Risk Level: Low (new unused feature)
Testing: Unit
Docs Changes: N/A (will be added when the admin endpoint is created)
Release Notes: N/A (likewise)

dnoe

A few nitpicky comments. The format check failed, I recommend running fix_format using the docker container to make sure you're always using the same version of clang-format as CI.

dnoe · 2018-01-04T02:36:29Z

source/common/runtime/runtime_impl.cc

+  std::unordered_map<std::string, const Snapshot::Entry*> entries;
+  entries.reserve(values_.size());
+
+  for (auto& kv : values_) {


Can it be const auto& kv?

dnoe · 2018-01-04T02:39:16Z

source/common/runtime/runtime_impl.h

@@ -190,6 +185,11 @@ class NullLoaderImpl : public Loader {
      return default_value;
    }

+    std::unordered_map<std::string, const Snapshot::Entry*> getAll() const override {
+      std::unordered_map<std::string, const Snapshot::Entry*> entries;


Could do just return std::unordered_map<std::string, const Snapshot::Entry*>(); - your call

I'll go with that. Much cleaner :)

dnoe · 2018-01-04T02:40:56Z

include/envoy/runtime/runtime.h

@@ -96,6 +113,12 @@ class Snapshot {
   * @return uint64_t the runtime value or the default value.
   */
  virtual uint64_t getInteger(const std::string& key, uint64_t default_value) const PURE;
+
+  /**
+   * Fetch the raw runtime entries map.


Can you add something to this comment about the lifespan of the const Entry*?

Signed-off-by: Chris Roche <[email protected]>

mattklein123

LGTM, thanks! Two small comments.

mattklein123 · 2018-01-04T05:57:03Z

include/envoy/runtime/runtime.h

+    /**
+     * The raw runtime data.
+     */
+    std::string string_value;


We are not fully consistent here, but I think most structs still use _ at the end of members, so I would probably change it back.

mattklein123 · 2018-01-04T06:00:49Z

include/envoy/runtime/runtime.h

+   * Fetch the raw runtime entries map. The map data is safe only for the lifetime of the Snapshot.
+   * @return std::unordered_map<std::string, const Entry*> the raw map of loaded runtime values.
+   */
+  virtual std::unordered_map<std::string, const Entry*> getAll() const PURE;


Is the reason you are copying here the const Entry vs. not const Entry? If that's the case, I would probably do two things:

I would use std::reference_wrapper to make it clear it can't be null.

I would probably return a vector of pairs, vs. a map. I don't think the caller will really ever need the map functionality, is they could otherwise call the snapshot directly if they need a single value.

P.S., I'm not sure but you might also be able to just make the internal map store const Entry. I'm not sure if this will work or not due to STL copy semantics, but it might. I would try that first.

Went with <string, const Entry> much nicer and the getAll can now return a const ref to the map instead. Much better!

…ntry> Signed-off-by: Chris Roche <[email protected]>

Signed-off-by: Chris Roche <[email protected]>

mattklein123

nice, much better. small comment then LGTM

mattklein123 · 2018-01-04T21:46:02Z

source/common/runtime/runtime_impl.cc

@@ -244,7 +249,8 @@ void SnapshotImpl::walkDirectory(const std::string& path, const std::string& pre
        entry.uint_value_.value(converted);
      }

-      values_[full_prefix] = entry;
+      values_.erase(full_prefix);


It's unfortunate that this now requires two hash lookups, but I think it's worth it as the interface is much better IMO. I think in C++17 http://en.cppreference.com/w/cpp/container/unordered_map/insert_or_assign could be used here, but I'm not totally sure. I would probably drop a small comment on why we are doing erase/insert here.

thought the same; I'd assume lightweight compared to the IO we're doing before it, though.

Comment added!

Signed-off-by: Chris Roche <[email protected]>

) * add basic metadata exchange Signed-off-by: Kuat Yessenov <[email protected]> * simplify Signed-off-by: Kuat Yessenov <[email protected]> * review feedback Signed-off-by: Kuat Yessenov <[email protected]>

rodaine force-pushed the runtime-get-all branch from b0b2f80 to f690729 Compare January 4, 2018 02:27

dnoe reviewed Jan 4, 2018

View reviewed changes

runtime: add getAll accessor to Snapshot to access underlying Entry map

67328c7

Signed-off-by: Chris Roche <[email protected]>

rodaine force-pushed the runtime-get-all branch from f690729 to 67328c7 Compare January 4, 2018 02:59

address CR comments

c95e4f0

Signed-off-by: Chris Roche <[email protected]>

dnoe previously approved these changes Jan 4, 2018

View reviewed changes

mattklein123 reviewed Jan 4, 2018

View reviewed changes

re-suffix struct members + convert underlying map to <string, const E…

0ea42cc

…ntry> Signed-off-by: Chris Roche <[email protected]>

rodaine dismissed dnoe’s stale review via 0ea42cc January 4, 2018 21:39

Chris Roche added 2 commits January 4, 2018 13:41

fix @return type

fe33763

Signed-off-by: Chris Roche <[email protected]>

fix @return type

32ee0e8

Signed-off-by: Chris Roche <[email protected]>

mattklein123 reviewed Jan 4, 2018

View reviewed changes

add comment about map double access

3cf4d5d

Signed-off-by: Chris Roche <[email protected]>

mattklein123 approved these changes Jan 4, 2018

View reviewed changes

mattklein123 merged commit 536fd63 into envoyproxy:master Jan 4, 2018

rodaine deleted the runtime-get-all branch January 4, 2018 23:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

runtime: add getAll accessor to Snapshot to access underlying Entry map #2311

runtime: add getAll accessor to Snapshot to access underlying Entry map #2311

rodaine commented Jan 4, 2018

dnoe left a comment

dnoe Jan 4, 2018

rodaine Jan 4, 2018

dnoe Jan 4, 2018

rodaine Jan 4, 2018

dnoe Jan 4, 2018

rodaine Jan 4, 2018

mattklein123 left a comment

mattklein123 Jan 4, 2018

rodaine Jan 4, 2018

mattklein123 Jan 4, 2018

mattklein123 Jan 4, 2018

rodaine Jan 4, 2018

mattklein123 left a comment

mattklein123 Jan 4, 2018

rodaine Jan 4, 2018

runtime: add getAll accessor to Snapshot to access underlying Entry map #2311

runtime: add getAll accessor to Snapshot to access underlying Entry map #2311

Conversation

rodaine commented Jan 4, 2018

dnoe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattklein123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattklein123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment