Support for publicly accessible configuration parsing to support testing #88

ACEmilG · 2018-03-23T21:07:24Z

No description provided.

bmoyles0117 · 2018-03-23T21:12:20Z

test/configuration_unittest.cc

-    EXPECT_EQ("", config.InstanceId());
-    EXPECT_EQ("", config.InstanceZone());
-  }
+static void ParseConfiguration(MetadataAgentConfiguration& config, const std::string& input) {


Instead of having this helper, is it possible to just call std::stringstream inline throughout? This way other tests don't try to recycle this method.

If not, I would vote for ParseConfigurationString, ParseConfigurationStream, and rename ParseConfig to ParseConfigurationFile.

igorpeshansky · 2018-03-23T22:40:08Z

src/configuration.h

  // value means that parsing succeeded, but all of the arguments were handled
  // within the function and the program should exit with a success exit code.
  int ParseArguments(int ac, char** av);
+  void ParseConfigurationStream(std::istream& input);


Instead, can we please just add a public constructor with a std::istream& parameter? Something like:

MetadataAgentConfiguration::MetadataAgentConfiguration(std::istream& input) : MetadataAgentConfiguration() { ParseConfiguration(input); }

and then construct the stream in the tests as:

const MetadataAgentConfiguration config(std::istringstream( "ProjectId: TestProjectId\n" "MetadataApiNumThreads: 13\n" "MetadataReporterPurgeDeleted: true\n" "MetadataReporterUserAgent: \"foobar/foobaz\"\n" ));

?

@igorpeshansky would using a constructor instead of a public method still allow us to dynamically reload configurations in the future? It would be nice if the metadata agent could pick up changes to the configuration on the disk and reload in the future, either via watching for changes on the file itself periodically, or by receiving a signal to reload the configuration. This could help us reduce the current negative impact of the metadata agent becoming unavailable during a restart, if the only intended change is a configuration update. Kubernetes already updates the files on disk when they're updated via ConfigMaps or Secrets, which would make this an awesome candidate. If you feel that a constructor / public method is just a matter of implementation, fine by me, as long as we are open to the possibility of dynamically reloading the configuration.

Adding to this, I know both "allow us", but what are your thoughts on the topic as a whole, and which approach would be more idiomatic in your mind?

I believe that such a functionality, when it does get added, should be encapsulated in the Configuration class. No external entity should be responsible for keeping the configuration up to date.

@igorpeshansky I agree with that principle, however its not represented in the current implementation. Right now the configuration class has an empty constructor and requires the something external to call ParseArguments(int ac, char** av).

Perhaps we should have 2 constructors: MetadataAgentConfiguration(std::istream& input) and MetadataAgentConfiguration(int ac, char ** av)?

I see, the problem is with error reporting. ParseArguments(int ac, char** av) needs to return the success mode to the caller. It does seem that this initialization needs to be done outside a constructor.

In the interest of moving things forward, I am going to implement the constructor as @igorpeshansky has described. Lets talk offline so I can better understand why this is the way to do it.

Offline conclusion: We will make int ParseArguments(int ac, char** av) private, and int ::main(int ac, char** av); a friend of MetadataAgentConfiguration. This friendship indicates an encapsulation leak that will be refactored out at a later point.

igorpeshansky

Some stylistic things.

igorpeshansky · 2018-03-26T18:12:00Z

src/configuration.h

 #include <string>

-namespace google {
+int main(int ac, char** av);


You shouldn't need to pre-declare it here.

I ran into a compilation issue without this forward declaration. Specifically: ‘int main(int, char**)’ should have been declared inside ‘::’

Yeah, you're right, it needs to know about the main in the global namespace.

igorpeshansky · 2018-03-26T18:12:34Z

src/configuration.h

-namespace google {
+int main(int ac, char** av);

+namespace google {


Let's keep the blank line between the namespace and the class.

igorpeshansky · 2018-03-26T18:13:09Z

src/configuration.h


-  void ParseConfigFile(const std::string& filename);
-  void ParseConfiguration(std::istream& input);
+  void ParseConfigurationStream(std::istream& input);


This renaming of private functions now seems gratuitous. Can we rename them back to their original names?

Sure, reverted.

igorpeshansky · 2018-03-26T18:15:00Z

test/configuration_unittest.cc

+  EXPECT_EQ("limit=30", config.DockerContainerFilter());
+  EXPECT_EQ(0, config.KubernetesUpdaterIntervalSeconds());
+  EXPECT_EQ("https://kubernetes.default.svc",
+            config.KubernetesEndpointHost());


Fits on the previous line.

igorpeshansky · 2018-03-26T18:15:31Z

test/configuration_unittest.cc

+  EXPECT_EQ("", config.InstanceResourceType());
+  EXPECT_EQ(60, config.DockerUpdaterIntervalSeconds());
+  EXPECT_EQ("unix://%2Fvar%2Frun%2Fdocker.sock/",
+            config.DockerEndpointHost());


Fits on the previous line.

igorpeshansky · 2018-03-26T18:16:52Z

test/configuration_unittest.cc

-  VerifyDefaultConfig();
+TEST(MetadataAgentConfigurationTest, EmptyConfig) {
+  std::istringstream stream("");
+  MetadataAgentConfiguration config(stream);


I would just inline the stream constructor, i.e.:

MetadataAgentConfiguration config(std::istringstream(""));

There seems to be some sort of compilation issue calling config with std::istringstream inline. Specifically: cannot bind non-const lvalue reference of type ‘std::istream& {aka std::basic_istream<char>&}’ to an rvalue of type ‘std::basic_istream<char>’

Ah, we would need to add a delegating constructor with an rvalue reference:

class MetadataAgentConfiguration { ... MetadataAgentConfiguration(std::istream&& input) : MetadataAgentConfiguration(input) {} ... };

because a temporary std::istringstream is an rvalue reference, and we need to explicitly convert it to an lvalue.

igorpeshansky · 2018-03-26T18:17:43Z

test/configuration_unittest.cc

      "MetadataReporterPurgeDeleted: true\n"
      "MetadataReporterUserAgent: \"foobar/foobaz\"\n"
  );
+  MetadataAgentConfiguration config(stream);


I would just inline the stream constructor, i.e.:

MetadataAgentConfiguration config(std::istringstream( "ProjectId: TestProjectId\n" "MetadataApiNumThreads: 13\n" "MetadataReporterPurgeDeleted: true\n" "MetadataReporterUserAgent: \"foobar/foobaz\"\n" ));

Also below.

igorpeshansky · 2018-03-26T18:20:15Z

src/configuration.h


 private:
-  friend class MetadataAgentConfigurationTest;
+  friend int ::main(int ac, char** av);


Let's add a comment on why it's a friend. You also shouldn't need the parameter names for a friend declaration. So, e.g.:

friend int ::main(int, char**); // Calls ParseArguments.

WDYT?

Agreed, changed.

igorpeshansky · 2018-03-26T20:26:04Z

src/configuration.h

 #include <string>

-namespace google {
+int main(int ac, char** av);


Yeah, you're right, it needs to know about the main in the global namespace.

igorpeshansky · 2018-03-26T20:28:27Z

src/configuration.h

 #include <mutex>
 #include <string>

+int main(int ac, char** av);


You can remove the parameter names here as well:

int main(int, char**);

igorpeshansky · 2018-03-26T20:29:13Z

src/configuration.h


-  void ParseConfigFile(const std::string& filename);
  void ParseConfiguration(std::istream& input);
+  void ParseConfigFile(const std::string& filename);


Might as well keep them in the original order, too. 😄

igorpeshansky · 2018-03-26T20:31:34Z

test/configuration_unittest.cc

-  VerifyDefaultConfig();
+TEST(MetadataAgentConfigurationTest, EmptyConfig) {
+  std::istringstream stream("");
+  MetadataAgentConfiguration config(stream);


Ah, we would need to add a delegating constructor with an rvalue reference:

class MetadataAgentConfiguration { ... MetadataAgentConfiguration(std::istream&& input) : MetadataAgentConfiguration(input) {} ... };

because a temporary std::istringstream is an rvalue reference, and we need to explicitly convert it to an lvalue.

igorpeshansky

LGTM

Don't forget to squash-and-merge.

…declaration of main

igorpeshansky · 2018-03-27T17:58:45Z

test/health_checker_unittest.cc

-  MetadataAgentConfiguration config_;
 };

+static const std::string IsolationPath(const std::string& test_name) {


const is superfluous here.

igorpeshansky · 2018-03-27T17:58:53Z

test/health_checker_unittest.cc

-  MetadataAgentConfiguration config_;
 };

+static const std::string IsolationPath(const std::string& test_name) {


IsolationPathConfig

igorpeshansky · 2018-03-27T17:59:27Z

test/health_checker_unittest.cc

-  MetadataAgentConfiguration config_;
 };

+static const std::string IsolationPath(const std::string& test_name) {


Why not just have this return std::istringstream?

bmoyles0117 · 2018-03-27T21:28:44Z

src/configuration.h

-  // within the function and the program should exit with a success exit code.
-  int ParseArguments(int ac, char** av);
+  MetadataAgentConfiguration(std::istream& input);
+  MetadataAgentConfiguration(std::istream&& input)


I'm not sure what's happening here, why do we need two constructors?

@ACEmilG added a comment with an explanation.

igorpeshansky

One more.

igorpeshansky · 2018-03-27T22:22:21Z

test/health_checker_unittest.cc

-  MetadataAgentConfiguration config_;
 };

+static std::istringstream IsolationPathConfig(const std::string& test_name) {


Forgot this one. static is ok within a class, but for a free-standing function, it's preferred to have it in an anonymous namespace (style guide).

static is no longer needed, right?

igorpeshansky · 2018-03-27T22:28:02Z

test/health_checker_unittest.cc

-  MetadataAgentConfiguration config_;
 };

+static std::istringstream IsolationPathConfig(const std::string& test_name) {


static is no longer needed, right?

igorpeshansky

LGTM

bmoyles0117

LGTM

bmoyles0117 reviewed Mar 23, 2018

View reviewed changes

igorpeshansky suggested changes Mar 23, 2018

View reviewed changes

igorpeshansky suggested changes Mar 26, 2018

View reviewed changes

igorpeshansky mentioned this pull request Mar 26, 2018

Add file health check support #85

Merged

igorpeshansky suggested changes Mar 26, 2018

View reviewed changes

igorpeshansky approved these changes Mar 26, 2018

View reviewed changes

ACEmilG added 6 commits March 27, 2018 13:12

Support for publicly accessible configuration parsing to support testing

b679920

addressed code review comments

efc5250

Support for constructor that takes istream

5bafe43

Move ParseArguments private to reflect encapsulation conversation

83798b9

Addressed code review comments

daa4de4

Support for inlining configuration stream, removed args from forward …

c0bf4ec

…declaration of main

ACEmilG force-pushed the ACEmilG-configuration-parsing branch from f5b5926 to c0bf4ec Compare March 27, 2018 17:27

igorpeshansky suggested changes Mar 27, 2018

View reviewed changes

bmoyles0117 reviewed Mar 27, 2018

View reviewed changes

Added clarifying comment to constructor

7d08a61

ACEmilG force-pushed the ACEmilG-configuration-parsing branch from edd9c4e to 7d08a61 Compare March 27, 2018 21:42

Addressed code review comments

72c3d58

ACEmilG force-pushed the ACEmilG-configuration-parsing branch from 8ead5cb to 72c3d58 Compare March 27, 2018 22:17

igorpeshansky suggested changes Mar 27, 2018

View reviewed changes

Added namespace for static free standing function

344fe46

igorpeshansky suggested changes Mar 27, 2018

View reviewed changes

Remove static from function in anonymous namespace

84ceff7

igorpeshansky approved these changes Mar 27, 2018

View reviewed changes

bmoyles0117 approved these changes Mar 27, 2018

View reviewed changes

ACEmilG merged commit ad9500c into master Mar 27, 2018

ACEmilG deleted the ACEmilG-configuration-parsing branch March 27, 2018 23:02

dhrupadb pushed a commit that referenced this pull request Mar 28, 2018

Add constructor that parses configuration to support testing. (#88)

163db9b

Support for publicly accessible configuration parsing to support testing #88

Support for publicly accessible configuration parsing to support testing #88

Uh oh!

Conversation

ACEmilG commented Mar 23, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

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!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ACEmilG Mar 26, 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

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!

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!

igorpeshansky Mar 23, 2018 •

edited

Loading

ACEmilG Mar 26, 2018 •

edited

Loading

ACEmilG Mar 26, 2018 •

edited by igorpeshansky

Loading