Update cpp-netlib to the latest (540ed7622be3f9534709036522f86bde1e84829f). by igorpeshansky · Pull Request #78 · Stackdriver/metadata-agent

igorpeshansky · 2018-03-13T23:45:29Z

No description provided.

bmoyles0117

A lot to chew on here. Is there a way of compartmentalizing the changes between the changes in the server implementation, and the change in the watch implementation? The server implementation updates are nice and easy to digest.

bmoyles0117 · 2018-03-14T15:00:28Z

src/asio/local_resolve_op.hpp

      else {
        //LOG(ERROR) << "async_resolve() " << o->query_.host_name();
-        std::string path = boost::network::uri::decoded(o->query_.host_name());
+        std::string path = ::network::detail::decode(o->query_.host_name());


Nooby question, can you point me to docs that explain the :: prefix?

http://en.cppreference.com/w/cpp/language/namespace#Using-declarations and http://en.cppreference.com/w/cpp/language/unqualified_lookup.

bmoyles0117 · 2018-03-14T15:03:41Z

src/http/local_async_normal.hpp

-      boost::enable_shared_from_this<
+      std::enable_shared_from_this<
          http_async_connection<Tag, version_major, version_minor> > {
+  http_async_connection(http_async_connection const&) = delete;


Why is this being set to delete?

This file is effectively a copy of boost/network/protocol/http/client/connection/async_normal.hpp -- I just pulled in the changes from the original. Let me move such changes out into a separate commit, for ease of reviewing. Stay tuned.

bmoyles0117 · 2018-03-14T15:10:12Z

src/http/local_async_normal.hpp

-  int timeout_;
-  boost::asio::deadline_timer timer_;
+  int64_t timeout_;
+  bool remove_chunk_markers_;


I'm not sure how much value this flag provides. Can you expand on why you're using it, instead of just leaving the conditional checks as if (this->is_chunk_encoding) { ?

bmoyles0117 · 2018-03-14T15:13:42Z

src/http/local_async_normal.hpp

+            // We short-circuit here because the user does not want to get the
+            // body (in the case of a HEAD request).
            this->body_promise.set_value("");
+            if ( callback )


Remove extra spacing around callback? Also, this is the first time I've noticed a single condition if without using the braces. If the readability guidelines are "choose what's best for you" around this, can we keep the braces? Single line conditionals have burned me so many times in the past.

Ditto here -- this is copied from boost/network/protocol/http/client/connection/async_normal.hpp.

bmoyles0117 · 2018-03-14T15:14:11Z

src/http/local_async_normal.hpp

+            // body (in the case of a HEAD request).
            this->body_promise.set_value("");
+            if ( callback )
+              callback( boost::iterator_range<typename std::array<typename char_<Tag>::type, 1024>::const_iterator>(), boost::asio::error::eof );


Extra spaces at the beginning and end of the invocation. Are these intentional?

bmoyles0117 · 2018-03-14T15:16:17Z

src/http/local_async_normal.hpp

+              }
            }
-            // TODO set the destination value somewhere!
+            // TODO(dberris): set the destination value somewhere!


Is dberris actually going to help here? If not, can we assign the todo to someone who will?

igorpeshansky

Sorry, realized that the context of these "copy-to-specialize" hacks keeps getting lost over time. I'll pull those out into a separate commit to help in reviewing. Will ping this PR when it's ready again.

igorpeshansky · 2018-03-14T16:14:20Z

src/asio/local_resolve_op.hpp

      else {
        //LOG(ERROR) << "async_resolve() " << o->query_.host_name();
-        std::string path = boost::network::uri::decoded(o->query_.host_name());
+        std::string path = ::network::detail::decode(o->query_.host_name());


http://en.cppreference.com/w/cpp/language/namespace#Using-declarations and http://en.cppreference.com/w/cpp/language/unqualified_lookup.

igorpeshansky · 2018-03-14T23:07:57Z

src/http/local_async_normal.hpp

-      boost::enable_shared_from_this<
+      std::enable_shared_from_this<
          http_async_connection<Tag, version_major, version_minor> > {
+  http_async_connection(http_async_connection const&) = delete;


This file is effectively a copy of boost/network/protocol/http/client/connection/async_normal.hpp -- I just pulled in the changes from the original. Let me move such changes out into a separate commit, for ease of reviewing. Stay tuned.

igorpeshansky · 2018-03-14T23:09:05Z

src/http/local_async_normal.hpp

+            // We short-circuit here because the user does not want to get the
+            // body (in the case of a HEAD request).
            this->body_promise.set_value("");
+            if ( callback )


Ditto here -- this is copied from boost/network/protocol/http/client/connection/async_normal.hpp.

igorpeshansky · 2018-03-14T23:09:14Z

src/http/local_async_normal.hpp

+            // body (in the case of a HEAD request).
            this->body_promise.set_value("");
+            if ( callback )
+              callback( boost::iterator_range<typename std::array<typename char_<Tag>::type, 1024>::const_iterator>(), boost::asio::error::eof );


igorpeshansky · 2018-03-14T23:09:38Z

src/http/local_async_normal.hpp

+              }
            }
-            // TODO set the destination value somewhere!
+            // TODO(dberris): set the destination value somewhere!


igorpeshansky · 2018-03-14T23:09:48Z

src/http/local_async_normal.hpp

-  int timeout_;
-  boost::asio::deadline_timer timer_;
+  int64_t timeout_;
+  bool remove_chunk_markers_;


igorpeshansky · 2018-03-15T00:27:01Z

Ok, I've just split out the commit to change the replicas (as well as added comments to ensure this relationship is documented). PTAL.

supriyagarg · 2018-03-14T15:44:57Z

src/kubernetes.cc

  http::client client(
-      http::client::options().openssl_certificate(SecretPath("ca.crt")));
+      http::client::options()
+      .remove_chunk_markers(false)


why is remove_chunk_markers set to false here?

supriyagarg · 2018-03-15T16:30:53Z

src/http/local_async_connection_base.ipp

 #define BOOST_NETWORK_PROTOCOL_HTTP_IMPL_LOCAL_ASYNC_CONNECTION_BASE_IPP_20170307

+// Note: This file is mostly a copy of
+// boost/network/protocol/http/client/connection/async_base.hpp,


Nope, hpp is correct. The upstream library only has a single file. For various linker-related reasons, we had to split out the specialized function into its own implementation file.

supriyagarg · 2018-03-15T16:32:47Z

src/http/local_async_connection_base.hpp

@@ -1,6 +1,10 @@
 #ifndef BOOST_NETWORK_PROTOCOL_HTTP_IMPL_LOCAL_ASYNC_CONNECTION_BASE_20170307


this commit only comments on 6 out of the 10 files under src/http. Please add the provenance comments to the remaining 4 files, including "src/http/traits/local_resolver.hpp"

The files that don't have the comments aren't replicas -- they are complete reimplementations that don't share enough code with the originals. The reason these files needed to be copied is because they weren't designed to be specialized -- I'll fix that upstream in a separate refactoring.

I see. Since all names start with local_ it sounded like they were all copies.

Yeah. local_ refers to "local sockets" (a.k.a., Unix domain sockets), rather than a "local copy".

igorpeshansky

@supriyagarg I couldn't respond to your comment in kubernetes.cc because you asked the question on a stale commit. We set remove_chunk_markers to false in WatchMaster because we currently have our own chunked encoding parser. I intend to remove that in a future PR and use the built-in functionality.
PTAL.

igorpeshansky · 2018-03-15T17:43:41Z

src/http/local_async_connection_base.ipp

 #define BOOST_NETWORK_PROTOCOL_HTTP_IMPL_LOCAL_ASYNC_CONNECTION_BASE_IPP_20170307

+// Note: This file is mostly a copy of
+// boost/network/protocol/http/client/connection/async_base.hpp,


Nope, hpp is correct. The upstream library only has a single file. For various linker-related reasons, we had to split out the specialized function into its own implementation file.

igorpeshansky · 2018-03-15T17:44:58Z

src/http/local_async_connection_base.hpp

@@ -1,6 +1,10 @@
 #ifndef BOOST_NETWORK_PROTOCOL_HTTP_IMPL_LOCAL_ASYNC_CONNECTION_BASE_20170307


The files that don't have the comments aren't replicas -- they are complete reimplementations that don't share enough code with the originals. The reason these files needed to be copied is because they weren't designed to be specialized -- I'll fix that upstream in a separate refactoring.

igorpeshansky · 2018-03-15T23:22:04Z

Updated to incorporate the latest fixes. PTAL.

supriyagarg

LGTM

igorpeshansky

Thanks for the review.

igorpeshansky · 2018-03-16T23:53:22Z

src/http/local_async_connection_base.hpp

@@ -1,6 +1,10 @@
 #ifndef BOOST_NETWORK_PROTOCOL_HTTP_IMPL_LOCAL_ASYNC_CONNECTION_BASE_20170307


Yeah. local_ refers to "local sockets" (a.k.a., Unix domain sockets), rather than a "local copy".

bmoyles0117

LGTM

Update cpp-netlib to e80404ec3222b0bd516ea4ad475c66c7222a4350.

cc0eac6

igorpeshansky requested review from bmoyles0117 and supriyagarg March 13, 2018 23:45

bmoyles0117 suggested changes Mar 14, 2018

View reviewed changes

igorpeshansky commented Mar 14, 2018

View reviewed changes

igorpeshansky force-pushed the igorp-cpp-netlib-latest branch 2 times, most recently from 2d72119 to e89a162 Compare March 15, 2018 00:15

igorpeshansky added 5 commits March 14, 2018 20:24

Bring specialized replicas in sync with the new originals.

b418949

Add provenance comments to explain the contents of specialization files.

e2450d2

Fixes to be compatible with the latest cpp-netlib.

a69fe53

Some MacOS changes.

e108693

Apparently you have to turn on chunk encoding parsing now.

f9b63f3

igorpeshansky force-pushed the igorp-cpp-netlib-latest branch from e89a162 to f9b63f3 Compare March 15, 2018 00:25

supriyagarg reviewed Mar 15, 2018

View reviewed changes

igorpeshansky commented Mar 15, 2018

View reviewed changes

igorpeshansky added 2 commits March 15, 2018 18:59

Update cpp-netlib to 540ed7622be3f9534709036522f86bde1e84829f.

c528649

No need to explicitly turn on remove_chunk_markers anymore.

d6ef6a3

igorpeshansky changed the title ~~Update cpp-netlib to the latest (e80404ec3222b0bd516ea4ad475c66c7222a4350).~~ Update cpp-netlib to the latest (540ed7622be3f9534709036522f86bde1e84829f). Mar 15, 2018

supriyagarg approved these changes Mar 16, 2018

View reviewed changes

igorpeshansky commented Mar 16, 2018

View reviewed changes

bmoyles0117 approved these changes Mar 17, 2018

View reviewed changes

igorpeshansky merged commit db00dab into master Mar 17, 2018

igorpeshansky deleted the igorp-cpp-netlib-latest branch March 17, 2018 16:41

igorpeshansky mentioned this pull request Mar 18, 2018

Fix up gitdir pointers in submodules on broken git versions. #82

Merged

bmoyles0117 mentioned this pull request Mar 28, 2018

Set headers when writing responses to properly return status code. #99

Merged

		@@ -1,6 +1,10 @@
		#ifndef BOOST_NETWORK_PROTOCOL_HTTP_IMPL_LOCAL_ASYNC_CONNECTION_BASE_20170307

Conversation

igorpeshansky commented Mar 13, 2018

Uh oh!

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

igorpeshansky Mar 14, 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!

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 14, 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!

igorpeshansky commented Mar 15, 2018

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!

supriyagarg Mar 16, 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!

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!

igorpeshansky commented Mar 15, 2018

Uh oh!

supriyagarg left a comment

Choose a reason for hiding this comment

Uh oh!

igorpeshansky left a comment

igorpeshansky Mar 14, 2018 •

edited

Loading

igorpeshansky Mar 14, 2018 •

edited

Loading

supriyagarg Mar 16, 2018 •

edited by igorpeshansky

Loading