Added absl::optional dependency to replace current optional implementation of envoy #2688

srikailash · 2018-02-28T22:15:50Z

Signed-off-by: sri kailash [email protected]
Description:
This is a work in progress pull request to replace our current optional.h with absl::optional.
refers to #2651.
Risk Level: Low
Testing: Added unit test until current implementation.

htuch

Thanks for getting this started.

htuch · 2018-03-01T03:24:18Z

test/common/common/abseil_optional_test.cc

+#include "gtest/gtest.h"
+
+namespace Envoy {
+TEST(abseil_optional , basic_operations) {


You can skip this I think, Abseil should have its own set of tests (plus, most of this is a pretty big violation of Envoy C++ style ;) )

Sure, added this for my own checks.

For your reference, here are the absl::optional<> tests: https://github.com/abseil/abseil-cpp/blob/master/absl/types/optional_test.cc

@ccaraman, Thanks!

htuch · 2018-03-01T03:24:55Z

include/envoy/common/optional.h

@@ -1,6 +1,7 @@
 #pragma once

 #include "envoy/common/exception.h"
+#include "absl/types/optional.h"


I rather folks just consume this directly across the tree, rather than having indirection here via our own wrapper header/build target.

ack, I see less code changes as a benefit of wrapping with some extra maintenance cost. Would like to know other reasons if any from you. Thanks!

Generally I think it's just clearer to use the actual direct includes. I think we'll eventually go with the C++17 std::optional, which absl::optional is supposed to be compatible with, so we'll just do a giant sed at that point.

htuch

I noticed you haven't replaced the actual uses of Optional yet; I would go ahead and do them all in this PR. It shouldn't be that much work, it's just a bit tedious.

htuch · 2018-03-01T21:45:04Z

source/common/router/config_impl.h

@@ -13,6 +13,7 @@
 #include "envoy/api/v2/rds.pb.h"
 #include "envoy/api/v2/route/route.pb.h"
 #include "envoy/common/optional.h"


Do you want to remove the old one in the same sweep?

Well, it's still a work in progress PR. I don't mind doing that either. Let me know if you want me to do that.

Sure, let's just do one giant PR, it should be fairly easy to review, as the transforms should all be mechanical. Thanks.

srikailash · 2018-03-03T12:36:09Z

Let me help you with this giant review process:
Bulk changes:
1. Optional -> abs::optional
2. Call to .valid() function need not be done as abs::optional variables itself evaluates to true/false
3. var.value(new_value) -> var = new_value
4. Removed all the includes of envoy/common/optional.h
5. Added “absl/types/optional.h”
Trivial changes (non-bulk):
1 . Removed tests to optional.h
2. Removed code of optional.h
Non trivial changes
1. Added conditional return to bool functions which return .valid() call for old optional
2. Included exception.h directly in shared_memory_hashset.h
3. Since cluster_impl_test expects an exception for noSdsConfig test, throwing exception code is added to cds_json.cc

htuch · 2018-03-05T10:26:46Z

@srikailash Thanks! This is a really nice cleanup. Can you get this to pass the failing CI checks? E.g. fix format, DCO, etc?

absl/types/optional.h Signed-off-by: sri kailash <[email protected]>

srikailash · 2018-03-05T22:16:31Z

@htuch, fixed all ci tests except format. Tried to fix it using "tools/check_format.py fix" but turns out it still fails format tests. Any other way to fix format? Thanks!

htuch · 2018-03-06T04:41:52Z

@srikailash did you run check_format under the Docker image? If not, I recommend you do, as you might have a different version of clang-format installed locally.

htuch

Looks good, some changes to make..

htuch · 2018-03-06T04:43:35Z

source/common/http/conn_manager_impl.cc

@@ -789,7 +789,7 @@ void ConnectionManagerImpl::startDrainSequence() {
 void ConnectionManagerImpl::ActiveStream::refreshCachedRoute() {
  Router::RouteConstSharedPtr route = snapped_route_config_->route(*request_headers_, stream_id_);
  request_info_.route_entry_ = route ? route->routeEntry() : nullptr;
-  cached_route_.value(std::move(route));
+  cached_route_ = (std::move(route));


Can we get rid of the extra parens at sites like this?

I saw this coming ;). will update these things as well.

htuch · 2018-03-06T04:46:18Z

source/common/config/cds_json.cc

@@ -140,6 +140,9 @@ void CdsJson::translateCluster(const Json::Object& json_cluster,
    cluster.set_type(envoy::api::v2::Cluster::ORIGINAL_DST);
  } else {
    ASSERT(string_type == "sds");
+    if(!eds_config.has_value()){


Missing spaces here...

htuch · 2018-03-06T04:46:47Z

source/common/config/cds_json.cc

@@ -140,6 +140,9 @@ void CdsJson::translateCluster(const Json::Object& json_cluster,
    cluster.set_type(envoy::api::v2::Cluster::ORIGINAL_DST);
  } else {
    ASSERT(string_type == "sds");
+    if(!eds_config.has_value()){
+      throw EnvoyException("fetching invalid Optional value");


In the description you mention an new exception in retry_state_impl.cc; I don't see that, but I see this.. is this what you meant?

Yea, updated the description. Sorry about that.

htuch · 2018-03-06T04:48:40Z

source/common/tracing/zipkin/zipkin_core_types.h

@@ -171,7 +171,7 @@ class Annotation : public ZipkinBase {
  /**
   * @return true if the endpoint attribute is set, or false otherwise.
   */
-  bool isSetEndpoint() const { return endpoint_.valid(); }
+  bool isSetEndpoint() const {  if(endpoint_){return true;} return false; }


This should just be return endpoint_.has_value();

htuch · 2018-03-06T04:57:25Z

@jmarantz can you take a pass on this when the next patches arrive?

Signed-off-by: sri kailash <[email protected]>

Accoding to htuch, the local ones doesn't work because of compatability issues between clang versions. Signed-off-by: sri kailash <[email protected]>

Signed-off-by: sri kailash <[email protected]>

srikailash · 2018-03-06T22:39:03Z

@htuch , @jmarantz fixed all changes requested in previous review. Apparently has_value() call timeouts for //test/integration:http2_integration_test on ci/circleci mac tests (https://circleci.com/gh/envoyproxy/envoy/30033?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link or please check last two commits). Thanks!

htuch · 2018-03-07T15:37:27Z

@srikailash the OS X timeout is a known flake, please ignore. You're going to have to merge master to resolve the conflicts, unfortunately due to the size of this PR it's going to have to be done on an ongoing basis until we merge.

htuch · 2018-03-07T15:38:28Z

source/common/config/bootstrap_json.cc

    Config::Utility::translateEdsConfig(
        *json_sds,
        *bootstrap.mutable_dynamic_resources()->mutable_deprecated_v1()->mutable_sds_config());
-    eds_config.value(bootstrap.dynamic_resources().deprecated_v1().sds_config());
+    eds_config = (bootstrap.dynamic_resources().deprecated_v1().sds_config());


Still some more surplus parens.

Will do a second pass over the changes to fix these. Thanks for the review.

htuch · 2018-03-07T15:40:24Z

source/common/tracing/zipkin/zipkin_core_types.h


  /**
   * @return true of the endpoint attribute has been set, or false otherwise.
   */
-  bool isSetEndpoint() const { return endpoint_.valid(); }
+  bool isSetEndpoint() const {
+    if (endpoint_) {


I assume you'll replace with has_value given the comment re: OS X flake? Thanks.

Sure, will just have to revert the last commit.

This one still needs fixing.

Signed-off-by: sri kailash <[email protected]>

srikailash · 2018-03-08T11:06:03Z

@htuch, i probably need some help to resolve conflicts for some files. Do you think copying file from upstream and then re-applying replacement changes would be a good idea?(I am new to big code bases). Thanks!

jmarantz · 2018-03-08T13:11:23Z

I have some general suggestions about how to make large scale refactors easier to push through.

Automate the changes if you can, via clang or sed or whatever, and review -- or at least describe it in the PR description, which might be easier to review than the results of the automation.
Try to make the changes incremental with every step obviously &/or proveably correct
Break down the changes into a few reviewable chunks at a time, which makes it easier to push through review/merge without conflicts.

For this case, I'm not sure about suggestion 1 above. Suggestion 2 might be a lot of work if you don't do suggestion 1, but a thought here is to:

start by adding absl::optional's interface into Envoy's Optional class, and converting call-sites over in reviewable swaths
When done, remove the class and replace it with a 'using' to alias it to absl::optional (which should be a tiny change)
Then do one another sweep through the codebase and change all the references, so you can finally remove the alias.

Each of these changes should be much easier to review and merge without conflicts.

But at a minimum, it doesn't seem like it's necessary for you to remove the existing Optional class all at once, so you could just revert the conflicting files and do more passes later to clean them up.

htuch · 2018-03-08T17:49:26Z

@jmarantz I asked for the all-in-one PR here, as it's fairly mechanical IMHO.

htuch · 2018-03-08T17:51:03Z

Ideally you have a script to automate the sed(or the like as suggested by @jmarantz), so you should be able to either resolve conflicts or start in a new client and do this. If not, you might want to do what you suggest.

fixed conflicts and reapplied changes on conflicting files. Signed-off-by: sri kailash <[email protected]>

Signed-off-by: sri kailash <[email protected]>

srikailash · 2018-03-10T22:06:22Z

test/common/common/BUILD

@@ -10,13 +10,6 @@ load(

 envoy_package()

-envoy_cc_fuzz_test(


Hello @htuch, fixed merge conflicts and made sure there are no extra braces.
Adding this test gives me this error " //test/common/common:base64_fuzz_test_lib: invalid label 'base64_corpus/.!31337!favicon.ico' in element 0 of attribute 'data' in 'cc_library' rule: invalid target name 'base64_corpus/.!31337!favicon.ico': target names may not contain '!'
". Any idea how to fix this? Thanks in advance!

Look at your diff below, it looks like you're trying to check-in some weird stuff like test/common/common/base64_corpus/.!31337!favicon.ico. Can you git rm these from your repo and try again? We do need to preserve this target :)

htuch

Looks good, this is a really nice cleanup. Can you look into the base64_fuzz_test isue and the remaining small comments? Hopefully can merge soon to avoid another significant set of conflict resolution at your end.

htuch · 2018-03-11T20:18:33Z

test/common/common/BUILD

@@ -10,13 +10,6 @@ load(

 envoy_package()

-envoy_cc_fuzz_test(


Look at your diff below, it looks like you're trying to check-in some weird stuff like test/common/common/base64_corpus/.!31337!favicon.ico. Can you git rm these from your repo and try again? We do need to preserve this target :)

htuch · 2018-03-11T20:20:04Z

source/common/config/cds_json.cc

@@ -140,6 +140,9 @@ void CdsJson::translateCluster(const Json::Object& json_cluster,
    cluster.set_type(envoy::api::v2::Cluster::ORIGINAL_DST);
  } else {
    ASSERT(string_type == "sds");
+    if (!eds_config.has_value()) {


Technically this can be if (!eds_config) I think.

htuch · 2018-03-11T20:22:41Z

source/common/router/config_impl.cc

-      const uint64_t old_value = hash.valid() ? ((hash.value() << 1) | (hash.value() >> 63)) : 0;
-      hash.value(old_value ^ new_hash.value());
+      const uint64_t old_value = hash ? ((hash.value() << 1) | (hash.value() >> 63)) : 0;
+      hash = (old_value ^ new_hash.value());


Nit: can remove parens here.

htuch · 2018-03-11T20:23:21Z

source/common/router/config_utility.h

-   *         or the HTTP status code from the route's redirect if specified,
-   *         or an empty Option otherwise.
+   * @return absl::optional<Http::Code> the HTTP status from the route's direct_response if
+   * specified, or the HTTP status code from the route's redirect if specified, or an empty Option


Nit: vestigial reference to Option.

htuch · 2018-03-11T20:23:55Z

source/common/tracing/zipkin/zipkin_core_types.h


  /**
   * @return true of the endpoint attribute has been set, or false otherwise.
   */
-  bool isSetEndpoint() const { return endpoint_.valid(); }
+  bool isSetEndpoint() const {
+    if (endpoint_) {


This one still needs fixing.

srikailash · 2018-03-11T23:32:01Z

@htuch , Thanks for the review. Fixed things mentioned by you.

Signed-off-by: sri kailash <[email protected]>

mattklein123

Thanks for doing this. This is really awesome. One small question from a quick skim.

mattklein123 · 2018-03-11T23:56:16Z

source/common/config/cds_json.cc

@@ -140,6 +140,9 @@ void CdsJson::translateCluster(const Json::Object& json_cluster,
    cluster.set_type(envoy::api::v2::Cluster::ORIGINAL_DST);
  } else {
    ASSERT(string_type == "sds");
+    if (!eds_config) {


When does this happen? It seems like this should not happen due to schema checks?

@mattklein123 , copying from what i mentioned earlier about this change.
"Since cluster_impl_test expects an exception for noSdsConfig test, throwing exception code is added to cds_json.cc." (our previous optional.h throws exception when trying to retrieve value from empty optional.)
Thanks for the review!

OK I guess that was a bad error message before. Since you are already in here, can you potentially change the error message to be more useful and change the test to use EXPECT_THROW_WITH_MESSAGE to check it? Thank you!

@mattklein123, fixed. please take a look. Thanks!

Signed-off-by: sri kailash <[email protected]>

mattklein123

Thanks! @htuch anything else?

htuch

Awesome cleanup, thanks muchly!

srikailash · 2018-03-13T19:17:02Z

Thanks for merging :) will pick up something else from issues now.

Some absl::optional changes missed due to not being merged with envoyproxy#2688. Signed-off-by: Harvey Tuch <[email protected]>

Some absl::optional changes missed due to not being merged with #2688. Signed-off-by: Harvey Tuch <[email protected]>

ggreenway assigned htuch Feb 28, 2018

htuch suggested changes Mar 1, 2018

View reviewed changes

This is pre-review version of replacing our optional.h with

5a5ea8f

absl/types/optional.h Signed-off-by: sri kailash <[email protected]>

htuch suggested changes Mar 6, 2018

View reviewed changes

htuch assigned jmarantz Mar 6, 2018

sri kailash added 4 commits March 6, 2018 14:09

Minor format changes and removed extra if-else code.

6836501

Signed-off-by: sri kailash <[email protected]>

Initial fix format using docker tool instead of check_format locally.

c86f35b

Accoding to htuch, the local ones doesn't work because of compatability issues between clang versions. Signed-off-by: sri kailash <[email protected]>

Minor styling fixes

1a17270

Signed-off-by: sri kailash <[email protected]>

test commit to check the cause of timeouts in ci/mac test

0f9c367

Signed-off-by: sri kailash <[email protected]>

htuch reviewed Mar 7, 2018

View reviewed changes

htuch mentioned this pull request Mar 7, 2018

flakey OS X integration test #2428

Closed

Tests failed due to flakey osx test, reapplying changes

b41648d

Signed-off-by: sri kailash <[email protected]>

sri kailash and others added 5 commits March 10, 2018 01:42

Merge remote-tracking branch 'upstream/master'

16ba788

fixed conflicts and reapplied changes on conflicting files. Signed-off-by: sri kailash <[email protected]>

Continuing cleanups

0064d6a

Signed-off-by: sri kailash <[email protected]>

Merge branch 'master' into master

ed2084b

Fixed format using docker

d28cbe3

Signed-off-by: sri kailash <[email protected]>

Removed extra braces and extra if validation.

286457e

Signed-off-by: sri kailash <[email protected]>

srikailash commented Mar 10, 2018

View reviewed changes

htuch suggested changes Mar 11, 2018

View reviewed changes

Fixed changes requested in review.

f127737

Signed-off-by: sri kailash <[email protected]>

mattklein123 reviewed Mar 12, 2018

View reviewed changes

Changing error message in case of noSdsConfig

0834f82

Signed-off-by: sri kailash <[email protected]>

mattklein123 approved these changes Mar 13, 2018

View reviewed changes

htuch approved these changes Mar 13, 2018

View reviewed changes

htuch merged commit 725a6a4 into envoyproxy:master Mar 13, 2018

htuch added a commit to htuch/envoy that referenced this pull request Mar 14, 2018

build: fix build break in envoyproxy#2790.

cb612b1

Some absl::optional changes missed due to not being merged with envoyproxy#2688. Signed-off-by: Harvey Tuch <[email protected]>

htuch mentioned this pull request Mar 14, 2018

build: fix build break in #2790. #2806

Merged

htuch added a commit that referenced this pull request Mar 14, 2018

build: fix build break in #2790. (#2806)

7d03b23

Some absl::optional changes missed due to not being merged with #2688. Signed-off-by: Harvey Tuch <[email protected]>

Added absl::optional dependency to replace current optional implementation of envoy #2688

Added absl::optional dependency to replace current optional implementation of envoy #2688

Conversation

srikailash commented Feb 28, 2018

htuch 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

srikailash Mar 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

htuch 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

srikailash commented Mar 3, 2018 • edited Loading

htuch commented Mar 5, 2018

srikailash commented Mar 5, 2018

htuch commented Mar 6, 2018

htuch 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

htuch commented Mar 6, 2018

srikailash commented Mar 6, 2018 • edited Loading

htuch commented Mar 7, 2018

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

srikailash commented Mar 8, 2018

jmarantz commented Mar 8, 2018

htuch commented Mar 8, 2018

htuch commented Mar 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

htuch 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

srikailash commented Mar 11, 2018

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

mattklein123 left a comment

Choose a reason for hiding this comment

htuch left a comment

Choose a reason for hiding this comment

srikailash commented Mar 13, 2018

srikailash Mar 1, 2018 •

edited

Loading

srikailash commented Mar 3, 2018 •

edited

Loading

srikailash commented Mar 6, 2018 •

edited

Loading