Miscellaneous cleanup. by igorpeshansky · Pull Request #43 · Stackdriver/metadata-agent

igorpeshansky · 2017-12-19T00:07:10Z

Throw an exception on parse errors.
Allow parsing JSON streams with multiple objects.
Factor out streaming operators for request/response headers.
Remove superfluous std::bind calls.

igorpeshansky · 2017-12-19T00:07:23Z

@supriyagarg, you might want to review as well.

bmoyles0117 · 2017-12-19T14:54:14Z

+  if (all_values.size() > 1) {
+    std::ostringstream out;
+    out << "Getting a single value out of " << all_values.size();
+    throw json::Exception(out.str());


Should this really be an exception instead a warning?

No, I want this to be an error. This really means an expectation mismatch, and I don't want to drop the other values with just a bit of log spam.

Previously we only had the option to fetch a single value and that would either return the value or return a nullptr. The contract has changed since then, is there any code that would be negatively impacted by this? If so, have those already been surrounded by the appropriate try / catch blocks?

Let's keep the two separate. Previously, we only queried endpoints that would return a single JSON object at a time, so this should be fully backward compatible. We also used to return nullptr in case of a parse error, and now we throw an exception, but all those uses have been updated as part of this PR.
To answer @qingling128's comment, the old API always parsed a single value. The new implementation parses a vector of values. However, when only a single value is requested, we want to validate the input by ensuring that the parsed vector is a singleton. I've updated the error message.

I'm a bit confused reading this part as well. So we take a vector and want to make sure it has exactly one element? If so, shouldn't the exception message Getting a single value out of be changed to Getting more than one value out of instead?

Thanks for changing the error message. LGTM now.

bmoyles0117 · 2017-12-19T14:55:00Z

-  static std::unique_ptr<Value> FromStream(std::istream& stream);
-  static std::unique_ptr<Value> FromString(const std::string& input);
+  static std::vector<std::unique_ptr<Value>> AllFromStream(
+      std::istream& stream) throw(Exception);


Can we limit the throw to a json::Exception?

This is json::Exception. This code is within the json namespace.

Ahh I was confused, why is it that we need to include json:: here despite also being in the json namespace?

metadata-agent/src/json.h

Line 250 in 1edb94f

throw json::Exception("There is no " + field + " in " + ToString());

That was a typo. Fixed (there and in json.cc).

igorpeshansky

Responded. PTAL.

igorpeshansky · 2017-12-19T16:45:40Z

+  if (all_values.size() > 1) {
+    std::ostringstream out;
+    out << "Getting a single value out of " << all_values.size();
+    throw json::Exception(out.str());


No, I want this to be an error. This really means an expectation mismatch, and I don't want to drop the other values with just a bit of log spam.

igorpeshansky · 2017-12-19T16:46:15Z

-  static std::unique_ptr<Value> FromStream(std::istream& stream);
-  static std::unique_ptr<Value> FromString(const std::string& input);
+  static std::vector<std::unique_ptr<Value>> AllFromStream(
+      std::istream& stream) throw(Exception);


This is json::Exception. This code is within the json namespace.

qingling128 · 2017-12-19T18:19:44Z

  yajl_handle handle = yajl_alloc(&callbacks, NULL, (void*) &builder);
  yajl_config(handle, yajl_allow_comments, 1);
+  yajl_config(handle, yajl_allow_multiple_values, 1);
+  //yajl_config(handle, yajl_allow_trailing_garbage, 1);


Are we commenting out these two lines on purpose? If so, maybe leave a comment for the reason and under what circumstances we might want to uncomment them?

Yes. These were robustness changes that would allow us to talk to less formalized APIs. I decided we should instead do some validation on the Kubernetes API, but if it turns out to be buggy or unstable, we may end up uncommenting these.
I thought that the option names (allow_trailing_garbage and dont_validate_strings) were self-descriptive enough that they didn't merit a separate comment.

Ah, I see. Now I understand.

qingling128 · 2017-12-19T18:22:12Z

+  if (all_values.size() > 1) {
+    std::ostringstream out;
+    out << "Getting a single value out of " << all_values.size();
+    throw json::Exception(out.str());


I'm a bit confused reading this part as well. So we take a vector and want to make sure it has exactly one element? If so, shouldn't the exception message Getting a single value out of be changed to Getting more than one value out of instead?

qingling128 · 2017-12-19T18:30:39Z

+
+namespace google {
+
+// Allow logging request headers.


What does "allow" mean in this context? The method seems straight forward though.

It means "enable" -- without this method, using log << req.headers() would fail to compile. However, "enable" is overloaded (e.g., could mean a config option), so I went with a more neutral word.

Ah, I see. Thanks for the clarification.

igorpeshansky

Addressed feedback. PTAL.

igorpeshansky · 2017-12-19T18:20:50Z

+  if (all_values.size() > 1) {
+    std::ostringstream out;
+    out << "Getting a single value out of " << all_values.size();
+    throw json::Exception(out.str());


Let's keep the two separate. Previously, we only queried endpoints that would return a single JSON object at a time, so this should be fully backward compatible. We also used to return nullptr in case of a parse error, and now we throw an exception, but all those uses have been updated as part of this PR.
To answer @qingling128's comment, the old API always parsed a single value. The new implementation parses a vector of values. However, when only a single value is requested, we want to validate the input by ensuring that the parsed vector is a singleton. I've updated the error message.

igorpeshansky · 2017-12-19T18:23:38Z

-  static std::unique_ptr<Value> FromStream(std::istream& stream);
-  static std::unique_ptr<Value> FromString(const std::string& input);
+  static std::vector<std::unique_ptr<Value>> AllFromStream(
+      std::istream& stream) throw(Exception);


That was a typo. Fixed (there and in json.cc).

igorpeshansky · 2017-12-19T18:33:43Z

+
+namespace google {
+
+// Allow logging request headers.


It means "enable" -- without this method, using log << req.headers() would fail to compile. However, "enable" is overloaded (e.g., could mean a config option), so I went with a more neutral word.

igorpeshansky · 2017-12-19T18:36:00Z

  yajl_handle handle = yajl_alloc(&callbacks, NULL, (void*) &builder);
  yajl_config(handle, yajl_allow_comments, 1);
+  yajl_config(handle, yajl_allow_multiple_values, 1);
+  //yajl_config(handle, yajl_allow_trailing_garbage, 1);


Yes. These were robustness changes that would allow us to talk to less formalized APIs. I decided we should instead do some validation on the Kubernetes API, but if it turns out to be buggy or unstable, we may end up uncommenting these.
I thought that the option names (allow_trailing_garbage and dont_validate_strings) were self-descriptive enough that they didn't merit a separate comment.

qingling128

LGTM.

qingling128 · 2017-12-19T20:08:04Z

+
+namespace google {
+
+// Allow logging request headers.


Ah, I see. Thanks for the clarification.

qingling128 · 2017-12-19T20:11:25Z

  yajl_handle handle = yajl_alloc(&callbacks, NULL, (void*) &builder);
  yajl_config(handle, yajl_allow_comments, 1);
+  yajl_config(handle, yajl_allow_multiple_values, 1);
+  //yajl_config(handle, yajl_allow_trailing_garbage, 1);


Ah, I see. Now I understand.

qingling128 · 2017-12-19T20:13:15Z

+  if (all_values.size() > 1) {
+    std::ostringstream out;
+    out << "Getting a single value out of " << all_values.size();
+    throw json::Exception(out.str());


Thanks for changing the error message. LGTM now.

bmoyles0117

LGTM 👍

supriyagarg · 2017-12-19T22:36:36Z

LGTM

igorpeshansky added 4 commits November 13, 2017 12:16

Throw an exception when unable to parse JSON.

43e0b34

Really support parsing JSON streams with multiple objects.

bcd4796

std::thread does an implicit bind.

7681105

Factor out streaming operators for request/response headers.

1edb94f

igorpeshansky requested review from bmoyles0117 and qingling128 December 19, 2017 00:07

bmoyles0117 reviewed Dec 19, 2017

View reviewed changes

igorpeshansky commented Dec 19, 2017

View reviewed changes

Remove redundant namespace qualifiers.

dea4fd0

qingling128 suggested changes Dec 19, 2017

View reviewed changes

Tweak error message.

abd09a4

igorpeshansky commented Dec 19, 2017

View reviewed changes

qingling128 approved these changes Dec 19, 2017

View reviewed changes

bmoyles0117 approved these changes Dec 19, 2017

View reviewed changes

igorpeshansky merged commit 357355b into master Dec 19, 2017

igorpeshansky deleted the igorp-miscellaneous-cleanup branch December 19, 2017 23:05

Conversation

igorpeshansky commented Dec 19, 2017

Uh oh!

igorpeshansky commented Dec 19, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bmoyles0117 Dec 19, 2017 • 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!

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!

bmoyles0117 Dec 19, 2017 • 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!

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!

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!

qingling128 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!

bmoyles0117 left a comment

Choose a reason for hiding this comment

Uh oh!

supriyagarg commented Dec 19, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

bmoyles0117 Dec 19, 2017 •

edited by igorpeshansky

Loading

bmoyles0117 Dec 19, 2017 •

edited by igorpeshansky

Loading