Factor MetadataStore, MetadataReporter, and MetadataAgent into their own files. #87

igorpeshansky · 2018-03-22T04:23:24Z

Note: this is a large refactoring, but I tried to keep each commit incremental. Please review one commit at a time.

rbuskens

Thanks for breaking up your changes into bite-sized chunks. A few minor comments.

rbuskens · 2018-03-22T11:31:19Z

src/api_server.h

+// Runs the metadata tasks.
+class MetadataAgent {
+ public:
+  using Metadata = MetadataStore::Metadata;


This doesn't seem to be used anywhere in the MetadataAgent class. Can we remove?

NM. I see you remove this in a later commit.

rbuskens · 2018-03-22T11:52:31Z

src/api_server.h

 #include <memory>

 #include "configuration.h"
+#include "reporter.h"


I don't think we need to include this header file here.

No longer relevant.

rbuskens · 2018-03-22T11:54:39Z

src/reporter.h

+
+namespace google {
+
+// Storage for the metadata mapping.


This comment isn't correct. Please fix.

Right. This does bring up an interesting question of whether we should reference the config/storage objects directly instead of lugging the agent pointer around. Let me try that instead.

rbuskens · 2018-03-22T11:56:15Z

src/agent.h

+#include <memory>
+
+#include "configuration.h"
+#include "reporter.h"


Would "class MetadataReporter" be enough?

Yes, it would. Done.

Replace includes by forward declarations in headers whenever possible.

igorpeshansky

Thanks for the review. PTAL.

igorpeshansky · 2018-03-22T19:58:25Z

src/api_server.h

 #include <memory>

 #include "configuration.h"
+#include "reporter.h"


No longer relevant.

igorpeshansky · 2018-03-22T20:01:39Z

src/agent.h

+#include <memory>
+
+#include "configuration.h"
+#include "reporter.h"


Yes, it would. Done.

igorpeshansky · 2018-03-22T20:04:50Z

src/reporter.h

+
+namespace google {
+
+// Storage for the metadata mapping.


Right. This does bring up an interesting question of whether we should reference the config/storage objects directly instead of lugging the agent pointer around. Let me try that instead.

bmoyles0117 · 2018-03-22T21:18:22Z

src/agent.h

+#ifndef AGENT_H_
+#define AGENT_H_
+
+//#include "config.h"


Let's remove unnecessary imports, same throughout.

This specific one was a "reminder" to set up autotools to make this more portable. It appears all over the place. Probably no sense in keeping it, but I'd rather do it as a separate cleanup to keep this PR focused.

bmoyles0117 · 2018-03-22T21:22:05Z

src/api_server.cc

-    std::lock_guard<std::mutex> lock(agent_.resource_mu_);
-    const auto result = agent_.resource_map_.find(id);
-    if (result == agent_.resource_map_.end()) {
+    std::lock_guard<std::mutex> lock(store_.resource_mu_);


I don't love the fact that the resource_mu_ is being handled outside of the store, can we setup getters to protect direct access of resource_mu_?

I don't love this either, and was thinking along the same lines, except in a different PR. Let's keep this one focused on the refactoring.

bmoyles0117 · 2018-03-22T21:25:47Z

src/api_server.cc

              << " headers: " << request.headers
              << " body: " << request.body;
  }
  if (request.method == "GET" && request.destination.find(kPrefix) == 0) {


It'd be awesome to call something like MonitoredResourceHandler here, so that we can easily start adding other handlers based on method / path such as HealthcheckHandler, then, each handler would be easily testable as well.

Good idea, bad timing. This is supposed to be a refactoring with a specific purpose — I'll do this in a follow-up PR if you don't mind.

bmoyles0117 · 2018-03-22T21:26:47Z

src/docker.cc

 #include <boost/network/protocol/http/client.hpp>
 #include <chrono>

+#include "configuration.h"


Is this line necessary? I only ask because it didn't exist before, and it doesn't seem like any lines added call out this as a requirement any more than the lines before this PR. Same question throughout.

It got moved from the header file.

bmoyles0117 · 2018-03-22T21:30:45Z

src/reporter.cc

+    }
+    std::this_thread::sleep_for(period_);
+  }
+  //LOG(INFO) << "Metadata reporter exiting";


I'd vote for uncommenting this, so I can create a log metric, but please remove if you disagree.

I'd like to keep this PR focused on the refactoring. Happy to do it in a follow-up PR — there are a few other places where extra logging would help.

FWIW, I think this specific bit follows an infinite loop, so will never be printed. It's commented out pending the TODO above the loop.

bmoyles0117 · 2018-03-22T21:33:27Z

src/reporter.cc

+        });
+    // TODO: This is probably all kinds of inefficient...
+    const int size = metadata_entry->ToString().size();
+    if (empty_size + size > limit_bytes) {


Just curious, do we expect this to happen often? Do you know what likely scenarios would exist where this would be exceeded frequently? I'll setup log metrics for this line, and try to reproduce the event, so that we can get a handle on frequency once I hear from you.

This is just defensive programming to catch individual metadata entries that would take the whole request over the size limit. We haven't seen any so far, but given that we include the raw output of an unrelated API, this isn't too outrageous of a scenario in my mind.

igorpeshansky

Thanks for the review. PTAL.

igorpeshansky · 2018-03-22T22:02:09Z

src/agent.h

+#ifndef AGENT_H_
+#define AGENT_H_
+
+//#include "config.h"


This specific one was a "reminder" to set up autotools to make this more portable. It appears all over the place. Probably no sense in keeping it, but I'd rather do it as a separate cleanup to keep this PR focused.

igorpeshansky · 2018-03-22T22:03:34Z

src/api_server.cc

              << " headers: " << request.headers
              << " body: " << request.body;
  }
  if (request.method == "GET" && request.destination.find(kPrefix) == 0) {


Good idea, bad timing. This is supposed to be a refactoring with a specific purpose — I'll do this in a follow-up PR if you don't mind.

igorpeshansky · 2018-03-22T22:06:19Z

src/api_server.cc

-    std::lock_guard<std::mutex> lock(agent_.resource_mu_);
-    const auto result = agent_.resource_map_.find(id);
-    if (result == agent_.resource_map_.end()) {
+    std::lock_guard<std::mutex> lock(store_.resource_mu_);


I don't love this either, and was thinking along the same lines, except in a different PR. Let's keep this one focused on the refactoring.

igorpeshansky · 2018-03-22T22:07:16Z

src/docker.cc

 #include <boost/network/protocol/http/client.hpp>
 #include <chrono>

+#include "configuration.h"


It got moved from the header file.

igorpeshansky · 2018-03-22T22:08:57Z

src/reporter.cc

+    }
+    std::this_thread::sleep_for(period_);
+  }
+  //LOG(INFO) << "Metadata reporter exiting";


I'd like to keep this PR focused on the refactoring. Happy to do it in a follow-up PR — there are a few other places where extra logging would help.

igorpeshansky · 2018-03-22T22:12:30Z

src/reporter.cc

+        });
+    // TODO: This is probably all kinds of inefficient...
+    const int size = metadata_entry->ToString().size();
+    if (empty_size + size > limit_bytes) {


This is just defensive programming to catch individual metadata entries that would take the whole request over the size limit. We haven't seen any so far, but given that we include the raw output of an unrelated API, this isn't too outrageous of a scenario in my mind.

rbuskens · 2018-03-23T00:37:20Z

LGTM.

ACEmilG · 2018-03-22T20:58:10Z

src/api_server.cc

      auth_(environment_),
      period_(period_s),
-      reporter_thread_(&MetadataReporter::ReportMetadata, this) {}
+      reporter_thread_([=]() { ReportMetadata(); }) {}


Nit: Can't this be [this] rather than [=]? That would seem to be tighter/more explicit to me. I know that this change basically belongs to another commit and thus my questioning it here is maybe not very useful.

It can, and probably should in the future. However, #74 used [=] consistently throughout, so I'm just following that style. If we decide to tighten these captures, we could do it in a separate cleanup (and at this point I'm not sure it's worth it).

ACEmilG · 2018-03-23T13:33:53Z

src/agent.h


 #include "configuration.h"
-#include "reporter.h"
 #include "store.h"


I believe that this could also be replaced with a forward declaration of MetadataStore here.

Not really -- we need the full definition of MetadataStore to declare a field of this type, otherwise we'll get a "field has incomplete type" compiler error.
We could, however, forward-declare MetadataAgentConfiguration, which I did.

bmoyles0117

LGTM

supriyagarg

LGTM

igorpeshansky

Responded to comments, PTAL.

igorpeshansky · 2018-03-23T17:14:57Z

src/agent.h


 #include "configuration.h"
-#include "reporter.h"
 #include "store.h"


Not really -- we need the full definition of MetadataStore to declare a field of this type, otherwise we'll get a "field has incomplete type" compiler error.
We could, however, forward-declare MetadataAgentConfiguration, which I did.

igorpeshansky · 2018-03-23T17:16:25Z

src/api_server.cc

      auth_(environment_),
      period_(period_s),
-      reporter_thread_(&MetadataReporter::ReportMetadata, this) {}
+      reporter_thread_([=]() { ReportMetadata(); }) {}


It can, and probably should in the future. However, #74 used [=] consistently throughout, so I'm just following that style. If we decide to tighten these captures, we could do it in a separate cleanup (and at this point I'm not sure it's worth it).

ACEmilG · 2018-03-23T19:07:39Z

LGTM

igorpeshansky added 7 commits March 21, 2018 21:05

Replace an std::bind instance missed in #74.

1ccb224

Factor out MetadataStore from MetadataAgent.

b2aed76

Keep all MetadataStore functions together.

1561cfa

Use MetadataStore::Metadata instead of MetadataAgent::Metadata.

8e9c078

Pull out MetadataStore into a separate file.

d6a9a88

Pull out MetadataReporter into a separate file.

e0338bb

Pull out MetadataAgent into a separate file.

029b06f

igorpeshansky requested review from bmoyles0117, rbuskens and supriyagarg March 22, 2018 04:23

rbuskens suggested changes Mar 22, 2018

View reviewed changes

Stop using MetadataAgent as a wrapper around config+store.

715ab18

Replace includes by forward declarations in headers whenever possible.

igorpeshansky commented Mar 22, 2018

View reviewed changes

bmoyles0117 suggested changes Mar 22, 2018

View reviewed changes

igorpeshansky commented Mar 22, 2018

View reviewed changes

rbuskens approved these changes Mar 23, 2018

View reviewed changes

ACEmilG reviewed Mar 23, 2018

View reviewed changes

bmoyles0117 approved these changes Mar 23, 2018

View reviewed changes

supriyagarg approved these changes Mar 23, 2018

View reviewed changes

Forward-declare MetadataAgentConfiguration in agent.h.

a9e65ad

igorpeshansky commented Mar 23, 2018

View reviewed changes

ACEmilG approved these changes Mar 23, 2018

View reviewed changes

igorpeshansky merged commit 114e2b4 into master Mar 23, 2018

igorpeshansky deleted the igorp-api_server-refactor branch March 23, 2018 19:45

Factor MetadataStore, MetadataReporter, and MetadataAgent into their own files. #87

Factor MetadataStore, MetadataReporter, and MetadataAgent into their own files. #87

Uh oh!

Conversation

igorpeshansky commented Mar 22, 2018

Uh oh!

rbuskens left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

igorpeshansky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

igorpeshansky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rbuskens commented Mar 23, 2018

Uh oh!

ACEmilG Mar 22, 2018 • edited by igorpeshansky Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

ACEmilG Mar 22, 2018 •

edited by igorpeshansky

Loading

ACEmilG Mar 23, 2018 •

edited by igorpeshansky

Loading

ACEmilG commented Mar 23, 2018 •

edited

Loading