Plumb absl::Status through connectivity state notifiers by yashykt · Pull Request #23480 · grpc/grpc

yashykt · 2020-07-14T21:42:21Z

Split off from #23313

This change is

markdroth

Thanks for doing this!

My comments are all minor things -- structurally, this looks very good.

Please let me know if you have any questions.

Reviewed 32 of 42 files at r1, 8 of 10 files at r2.
Reviewable status: all files reviewed, 26 unresolved discussions (waiting on @yashykt)

src/core/ext/filters/client_channel/client_channel.cc, line 1957 at r2 (raw file):

      UpdateStateAndPickerLocked(
          GRPC_CHANNEL_SHUTDOWN,
          absl::Status(absl::StatusCode::kUnavailable,

We don't need to set status when transitioning to SHUTDOWN. It will only be used in TRANSIENT_FAILURE.

src/core/ext/filters/client_channel/lb_policy.h, line 27 at r2 (raw file):

#include <iterator>

#include "absl/strings/string_view.h"

Please add an include for absl/status/status.h here.

src/core/ext/filters/client_channel/resolving_lb_policy.cc, line 219 at r2 (raw file):

    channel_control_helper()->UpdateState(
        GRPC_CHANNEL_TRANSIENT_FAILURE,
        absl::Status(absl::StatusCode::kUnavailable,

This should get the status code from state_error using grpc_error_get_status() rather than hard-coding UNAVAILABLE.

src/core/ext/filters/client_channel/subchannel.cc, line 349 at r2 (raw file):

            c->channelz_node()->SetChildSocket(nullptr);
          }
          c->SetConnectivityStateLocked(GRPC_CHANNEL_TRANSIENT_FAILURE, status);

If the state is SHUTDOWN, then we'll need to construct our own Status here, because the one that was passed in will be OK, but we need a non-OK status when we set the state to TRANSIENT_FAILURE.

src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc, line 333 at r2 (raw file):

        // fallback mode immediately.
        gpr_log(GPR_INFO,
                "[grpclb %p] balancer channel in state:TRANSIENT_FAILURE "

Suggest formatting this as balancer channel in state TRANSIENT_FAILURE (%s); entering fallback mode.

src/core/ext/filters/client_channel/lb_policy/priority/priority.cc, line 664 at r2 (raw file):

    OnConnectivityStateUpdateLocked(
        GRPC_CHANNEL_TRANSIENT_FAILURE,
        absl::Status(absl::StatusCode::kUnavailable, grpc_error_string(error)),

In this case, grpc_error_string(error) won't return the right thing, since that will typically be GRPC_ERROR_NONE when the timer fires. We'll probably want to hard-code a message here. Let's set this to "failover timer fired".

src/core/ext/filters/client_channel/lb_policy/weighted_target/weighted_target.cc, line 181 at r2 (raw file):

    RefCountedPtr<ChildPickerWrapper> picker_wrapper_;
    grpc_connectivity_state connectivity_state_ = GRPC_CHANNEL_CONNECTING;
    absl::Status connectivity_status_;

This isn't actually going to be used anywhere right now, so there's probably no point in storing it.

src/core/ext/filters/client_channel/lb_policy/weighted_target/weighted_target.cc, line 380 at r2 (raw file):

  }
  std::unique_ptr<SubchannelPicker> picker;
  const char* error_string = nullptr;

Instead of having a const char* here, let's have an absl::Status. We can then set it to a non-OK value in the TRANSIENT_FAILURE case below.

src/core/ext/filters/client_channel/lb_policy/weighted_target/weighted_target.cc, line 393 at r2 (raw file):

      error_string =
          "weighted_target: all children report state TRANSIENT_FAILURE";
      absl::make_unique<TransientFailurePicker>(

Looks like you accidentally deleted the picker = in front of this expression.

src/core/ext/filters/client_channel/lb_policy/xds/cds.cc, line 213 at r2 (raw file):

    parent_->channel_control_helper()->UpdateState(
        GRPC_CHANNEL_TRANSIENT_FAILURE,
        absl::Status(absl::StatusCode::kUnavailable, grpc_error_string(error)),

Let's get the status code from the error via grpc_error_get_satus() instead of hard-coding UNAVAILABLE.

src/core/ext/filters/client_channel/lb_policy/xds/cds.cc, line 225 at r2 (raw file):

          "TRANSIENT_FAILURE",
          parent_.get(), parent_->config_->cluster().c_str());
  grpc_error* error = GRPC_ERROR_CREATE_FROM_COPIED_STRING(

Looks like there's a pre-existing bug here: this error should have an attached status of UNAVAILABLE.

src/core/ext/filters/client_channel/lb_policy/xds/cds.cc, line 231 at r2 (raw file):

  parent_->channel_control_helper()->UpdateState(
      GRPC_CHANNEL_TRANSIENT_FAILURE,
      absl::Status(absl::StatusCode::kUnavailable, grpc_error_string(error)),

Let's get the status code from the error via grpc_error_get_satus() instead of hard-coding UNAVAILABLE.

src/core/ext/filters/client_channel/lb_policy/xds/eds.cc, line 349 at r2 (raw file):

      eds_policy_->channel_control_helper()->UpdateState(
          GRPC_CHANNEL_TRANSIENT_FAILURE,
          absl::Status(absl::StatusCode::kUnavailable,

Let's get the status code from the error via grpc_error_get_satus() instead of hard-coding UNAVAILABLE.

src/core/ext/filters/client_channel/lb_policy/xds/eds.cc, line 363 at r2 (raw file):

        eds_policy_.get());
    grpc_error* error =
        GRPC_ERROR_CREATE_FROM_STATIC_STRING("EDS resource does not exist");

Looks like there's a pre-existing bug here: this error should have an attached status of UNAVAILABLE.

src/core/ext/filters/client_channel/lb_policy/xds/eds.cc, line 366 at r2 (raw file):

    eds_policy_->channel_control_helper()->UpdateState(
        GRPC_CHANNEL_TRANSIENT_FAILURE,
        absl::Status(absl::StatusCode::kUnavailable, grpc_error_string(error)),

Let's get the status code from the error via grpc_error_get_satus() instead of hard-coding UNAVAILABLE.

src/core/ext/filters/client_channel/lb_policy/xds/eds.cc, line 701 at r2 (raw file):

    channel_control_helper()->UpdateState(
        GRPC_CHANNEL_TRANSIENT_FAILURE,
        absl::Status(absl::StatusCode::kUnavailable, grpc_error_string(error)),

Let's get the status code from the error via grpc_error_get_satus() instead of hard-coding UNAVAILABLE. (In this case, it's actually supposed to be INTERNAL.)

src/core/ext/filters/client_channel/lb_policy/xds/xds_routing.cc, line 511 at r2 (raw file):

          GRPC_ERROR_INT_GRPC_STATUS, GRPC_STATUS_UNAVAILABLE));
  }
  channel_control_helper()->UpdateState(connectivity_state, absl::Status(),

This should compute the status based on the case above, just like you're doing in the weighted_target policy.

src/core/ext/transport/chttp2/transport/chttp2_transport.cc, line 570 at r2 (raw file):

        t, GRPC_CHANNEL_SHUTDOWN,
        absl::Status(
            absl::StatusCode::kUnavailable,

We don't need to send a status in this case, because the state is SHUTDOWN, not TRANSIENT_FAILURE.

src/core/ext/transport/chttp2/transport/chttp2_transport.cc, line 1115 at r2 (raw file):

                                       KEEPALIVE_TIME_BACKOFF_MULTIPLIER);
  }
  absl::Status status = absl::Status(absl::StatusCode::kUnavailable,

Let's get the status code from t->goaway_error via grpc_error_get_satus() instead of hard-coding UNAVAILABLE.

The message should also come from t->goaway_error.

src/core/ext/transport/inproc/inproc_transport.cc, line 1137 at r2 (raw file):

  t->state_tracker.SetState(
      GRPC_CHANNEL_SHUTDOWN,
      absl::Status(absl::StatusCode::kUnavailable, "Transport closed"),

No need to send a non-OK status here, since this is SHUTDOWN, not TRANSIENT_FAILURE.

src/core/lib/transport/connectivity_state.cc, line 63 at r2 (raw file):

           grpc_connectivity_state state,
           const std::shared_ptr<WorkSerializer>& work_serializer,
           const absl::Status& status)

Please move this parameter to be right after state, since the two parameters are both the content of the notification.

src/core/lib/transport/connectivity_state.cc, line 80 at r2 (raw file):

    Notifier* self = static_cast<Notifier*>(arg);
    if (GRPC_TRACE_FLAG_ENABLED(grpc_connectivity_state_trace)) {
      gpr_log(GPR_INFO, "watcher %p: delivering async notification for %s",

Might be a good idea to also include self->status_.ToString() in the log message here.

test/core/transport/connectivity_state_test.cc, line 86 at r2 (raw file):

}

TEST(StateTracker, NotificationUponAddingWatcher) {

Please add a variant of this where the tracker is set to TRANSIENT_FAILURE with a non-OK status, to make sure that gets delivered properly when a watcher is added.

test/core/transport/connectivity_state_test.cc, line 110 at r2 (raw file):

  absl::Status transient_failure_status(absl::StatusCode::kUnavailable,
                                        "status for testing");
  tracker.SetState(GRPC_CHANNEL_CONNECTING, transient_failure_status, "whee");

Let's change the state here to TRANSIENT_FAILURE, just to be consistent with the fact that we're sending a non-OK status.

test/core/transport/connectivity_state_test.cc, line 157 at r2 (raw file):

  absl::Status transient_failure_status(absl::StatusCode::kUnavailable,
                                        "status for testing");
  tracker.SetState(GRPC_CHANNEL_SHUTDOWN, transient_failure_status,

No need to set a non-OK status for SHUTDOWN.

test/core/transport/connectivity_state_test.cc, line 172 at r2 (raw file):

  absl::Status transient_failure_status(absl::StatusCode::kUnavailable,
                                        "status for testing");
  ConnectivityStateTracker tracker("xxx", GRPC_CHANNEL_SHUTDOWN,

Same here.

yashykt

Reviewable status: 24 of 42 files reviewed, 26 unresolved discussions (waiting on @markdroth and @yashykt)

src/core/ext/filters/client_channel/client_channel.cc, line 1957 at r2 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

We don't need to set status when transitioning to SHUTDOWN. It will only be used in TRANSIENT_FAILURE.

Done.

src/core/ext/filters/client_channel/lb_policy.h, line 27 at r2 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

Please add an include for absl/status/status.h here.

Done.

src/core/ext/filters/client_channel/resolving_lb_policy.cc, line 219 at r2 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

This should get the status code from state_error using grpc_error_get_status() rather than hard-coding UNAVAILABLE.

Done.

src/core/ext/filters/client_channel/subchannel.cc, line 349 at r2 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

If the state is SHUTDOWN, then we'll need to construct our own Status here, because the one that was passed in will be OK, but we need a non-OK status when we set the state to TRANSIENT_FAILURE.

you are right

src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc, line 333 at r2 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

Suggest formatting this as balancer channel in state TRANSIENT_FAILURE (%s); entering fallback mode.

I like it. Adopting this format everywhere

src/core/ext/filters/client_channel/lb_policy/priority/priority.cc, line 664 at r2 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

In this case, grpc_error_string(error) won't return the right thing, since that will typically be GRPC_ERROR_NONE when the timer fires. We'll probably want to hard-code a message here. Let's set this to "failover timer fired".

changed to the error message above. I think that would be more useful

src/core/ext/filters/client_channel/lb_policy/weighted_target/weighted_target.cc, line 181 at r2 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

This isn't actually going to be used anywhere right now, so there's probably no point in storing it.

Done.

src/core/ext/filters/client_channel/lb_policy/weighted_target/weighted_target.cc, line 380 at r2 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

Instead of having a const char* here, let's have an absl::Status. We can then set it to a non-OK value in the TRANSIENT_FAILURE case below.

Done.

src/core/ext/filters/client_channel/lb_policy/weighted_target/weighted_target.cc, line 393 at r2 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

Looks like you accidentally deleted the picker = in front of this expression.

Thanks!

src/core/ext/filters/client_channel/lb_policy/xds/cds.cc, line 213 at r2 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

Let's get the status code from the error via grpc_error_get_satus() instead of hard-coding UNAVAILABLE.

is there any case where it will not be unavailable?

src/core/ext/filters/client_channel/lb_policy/xds/cds.cc, line 225 at r2 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

Looks like there's a pre-existing bug here: this error should have an attached status of UNAVAILABLE.

Done.

src/core/ext/filters/client_channel/lb_policy/xds/eds.cc, line 349 at r2 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

Let's get the status code from the error via grpc_error_get_satus() instead of hard-coding UNAVAILABLE.

Done.

src/core/ext/filters/client_channel/lb_policy/xds/eds.cc, line 363 at r2 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

Looks like there's a pre-existing bug here: this error should have an attached status of UNAVAILABLE.

Done.

src/core/ext/filters/client_channel/lb_policy/xds/eds.cc, line 366 at r2 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

Let's get the status code from the error via grpc_error_get_satus() instead of hard-coding UNAVAILABLE.

Done.

src/core/ext/filters/client_channel/lb_policy/xds/eds.cc, line 701 at r2 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

Let's get the status code from the error via grpc_error_get_satus() instead of hard-coding UNAVAILABLE. (In this case, it's actually supposed to be INTERNAL.)

Done.

src/core/ext/filters/client_channel/lb_policy/xds/xds_routing.cc, line 511 at r2 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

This should compute the status based on the case above, just like you're doing in the weighted_target policy.

Done.

src/core/ext/transport/chttp2/transport/chttp2_transport.cc, line 570 at r2 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

We don't need to send a status in this case, because the state is SHUTDOWN, not TRANSIENT_FAILURE.

Done.

src/core/ext/transport/chttp2/transport/chttp2_transport.cc, line 1115 at r2 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

Let's get the status code from t->goaway_error via grpc_error_get_satus() instead of hard-coding UNAVAILABLE.

The message should also come from t->goaway_error.

Done.

src/core/ext/transport/inproc/inproc_transport.cc, line 1137 at r2 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

No need to send a non-OK status here, since this is SHUTDOWN, not TRANSIENT_FAILURE.

Done.

src/core/lib/transport/connectivity_state.cc, line 63 at r2 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

Please move this parameter to be right after state, since the two parameters are both the content of the notification.

Done.

src/core/lib/transport/connectivity_state.cc, line 80 at r2 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

Might be a good idea to also include self->status_.ToString() in the log message here.

Done.

test/core/transport/connectivity_state_test.cc, line 86 at r2 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

Please add a variant of this where the tracker is set to TRANSIENT_FAILURE with a non-OK status, to make sure that gets delivered properly when a watcher is added.

Done.

test/core/transport/connectivity_state_test.cc, line 110 at r2 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

Let's change the state here to TRANSIENT_FAILURE, just to be consistent with the fact that we're sending a non-OK status.

Done.

test/core/transport/connectivity_state_test.cc, line 157 at r2 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

No need to set a non-OK status for SHUTDOWN.

Done.

test/core/transport/connectivity_state_test.cc, line 172 at r2 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

Same here.

Done.

yashykt

Reviewable status: 24 of 42 files reviewed, 26 unresolved discussions (waiting on @markdroth)

src/core/ext/filters/client_channel/lb_policy/xds/cds.cc, line 231 at r2 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

Let's get the status code from the error via grpc_error_get_satus() instead of hard-coding UNAVAILABLE.

Done.

markdroth

This looks great! Just one suggestion remaining.

Reviewed 16 of 18 files at r3, 1 of 1 files at r4, 1 of 1 files at r5.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @yashykt)

src/core/ext/filters/client_channel/lb_policy/xds/cds.cc, line 213 at r2 (raw file):

Previously, yashykt (Yash Tibrewal) wrote…

is there any case where it will not be unavailable?

Probably not in practice, but our convention for this part of the code is that the original source of the error determines the resulting status code. If we ever add a case where the status code is something other than UNAVAILABLE, we'd set it where the error originates, and we wouldn't remember to change the code here.

src/core/lib/transport/error_utils.h, line 43 at r5 (raw file):

/// Utility Function to just return the status code given an \a error.
absl::StatusCode grpc_error_get_status_code(grpc_error* error);

How about changing this to return the full absl::Status? That might be even more useful as we transition from grpc_error to absl::Status internally.

yashykt

Reviewable status: 34 of 42 files reviewed, 1 unresolved discussion (waiting on @markdroth)

src/core/ext/filters/client_channel/lb_policy/xds/cds.cc, line 213 at r2 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

Probably not in practice, but our convention for this part of the code is that the original source of the error determines the resulting status code. If we ever add a case where the status code is something other than UNAVAILABLE, we'd set it where the error originates, and we wouldn't remember to change the code here.

Makes sense

src/core/lib/transport/error_utils.h, line 43 at r5 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

How about changing this to return the full absl::Status? That might be even more useful as we transition from grpc_error to absl::Status internally.

Converted to grpc_error_to_absl_status. If we ever get to C++ifying this we would probably want something like grpc_core::Error::ToAbslStatus()

markdroth

I did one more pass from the ground up. Still looks very good overall!

Now that we have grpc_error_to_absl_status(), we should use it in a lot more places.

Please let me know if you have any questions.

Reviewed 8 of 8 files at r6.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @yashykt)

src/core/ext/filters/client_channel/subchannel.cc, line 1057 at r6 (raw file):

      c->SetConnectivityStateLocked(
          GRPC_CHANNEL_TRANSIENT_FAILURE,
          absl::Status(

This should probably use grpc_error_to_absl_status().

src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc, line 204 at r6 (raw file):

    channel_control_helper()->UpdateState(
        GRPC_CHANNEL_TRANSIENT_FAILURE,
        absl::Status(absl::StatusCode::kUnavailable, grpc_error_string(error)),

This can use grpc_error_to_absl_status().

src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc, line 323 at r6 (raw file):

        p->channel_control_helper()->UpdateState(
            GRPC_CHANNEL_TRANSIENT_FAILURE,
            absl::Status(absl::StatusCode::kUnavailable,

This can use grpc_error_to_absl_status().

src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc, line 404 at r6 (raw file):

          p->channel_control_helper()->UpdateState(
              GRPC_CHANNEL_TRANSIENT_FAILURE,
              absl::Status(absl::StatusCode::kUnavailable,

This can use grpc_error_to_absl_status().

src/core/ext/filters/client_channel/lb_policy/priority/priority.cc, line 118 at r6 (raw file):

    }

    absl::Status connectivity_status() const { return connectivity_status_; }

This can return a const reference.

src/core/ext/filters/client_channel/lb_policy/priority/priority.cc, line 468 at r6 (raw file):

  channel_control_helper()->UpdateState(
      GRPC_CHANNEL_TRANSIENT_FAILURE,
      absl::Status(absl::StatusCode::kUnavailable, grpc_error_string(error)),

This can use grpc_error_to_absl_status().

src/core/ext/filters/client_channel/lb_policy/priority/priority.cc, line 656 at r6 (raw file):

      !priority_policy_->shutting_down_) {
    std::string error_message = absl::StrFormat(
        "[priority_lb %p] child %s (%p): failover timer fired, "

The [priority_lb %p] child %s (%p): part should probably be part of the trace log message but not part of the status message.

src/core/ext/filters/client_channel/lb_policy/round_robin/round_robin.cc, line 339 at r6 (raw file):

    p->channel_control_helper()->UpdateState(
        GRPC_CHANNEL_TRANSIENT_FAILURE,
        absl::Status(absl::StatusCode::kUnavailable, grpc_error_string(error)),

This can use grpc_error_to_absl_status().

src/core/ext/filters/client_channel/lb_policy/round_robin/round_robin.cc, line 457 at r6 (raw file):

    channel_control_helper()->UpdateState(
        GRPC_CHANNEL_TRANSIENT_FAILURE,
        absl::Status(absl::StatusCode::kUnavailable, grpc_error_string(error)),

This can use grpc_error_to_absl_status().

src/core/ext/filters/client_channel/lb_policy/weighted_target/weighted_target.cc, line 394 at r6 (raw file):

          "weighted_target: all children report state TRANSIENT_FAILURE");
      picker = absl::make_unique<TransientFailurePicker>(
          GRPC_ERROR_CREATE_FROM_COPIED_STRING(status.ToString().c_str()));

This looks like a pre-existing bug, but this error should have UNAVAILABLE status associated with it.

I suggest writing this similar to what we do elsewhere: create the error, set status from the error using grpc_error_to_absl_status(), and then pass the error to the TransientFailurePicker.

src/core/lib/transport/error_utils.h, line 43 at r5 (raw file):

Previously, yashykt (Yash Tibrewal) wrote…

Converted to grpc_error_to_absl_status. If we ever get to C++ifying this we would probably want something like grpc_core::Error::ToAbslStatus()

At this point, I doubt we'll ever C++-ify grpc_error. Instead, we'll probably just replace it with absl::Status everywhere.

src/core/lib/transport/error_utils.cc, line 132 at r6 (raw file):

                        nullptr /* error_string */);
  return absl::Status(static_cast<absl::StatusCode>(status),
                      grpc_error_string(error));

This should probably use the slice returned from grpc_error_get_status() rather than the whole error string.

yashykt

Reviewable status: 36 of 42 files reviewed, 11 unresolved discussions (waiting on @markdroth)

src/core/ext/filters/client_channel/subchannel.cc, line 1057 at r6 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

This should probably use grpc_error_to_absl_status().

Done.

src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc, line 204 at r6 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

This can use grpc_error_to_absl_status().

Done.

src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc, line 323 at r6 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

This can use grpc_error_to_absl_status().

Done.

src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc, line 404 at r6 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

This can use grpc_error_to_absl_status().

Done.

src/core/ext/filters/client_channel/lb_policy/priority/priority.cc, line 118 at r6 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

This can return a const reference.

Done.

src/core/ext/filters/client_channel/lb_policy/priority/priority.cc, line 468 at r6 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

This can use grpc_error_to_absl_status().

Done.

src/core/ext/filters/client_channel/lb_policy/priority/priority.cc, line 656 at r6 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

The [priority_lb %p] child %s (%p): part should probably be part of the trace log message but not part of the status message.

I thought about it, but couldn't think of a reason to remove it. I would imagine that it would be better to have more information if we are printing this status somewhere else so that we can trace it.

src/core/ext/filters/client_channel/lb_policy/round_robin/round_robin.cc, line 339 at r6 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

This can use grpc_error_to_absl_status().

Done.

src/core/ext/filters/client_channel/lb_policy/round_robin/round_robin.cc, line 457 at r6 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

This can use grpc_error_to_absl_status().

Done.

src/core/ext/filters/client_channel/lb_policy/weighted_target/weighted_target.cc, line 394 at r6 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

This looks like a pre-existing bug, but this error should have UNAVAILABLE status associated with it.

I suggest writing this similar to what we do elsewhere: create the error, set status from the error using grpc_error_to_absl_status(), and then pass the error to the TransientFailurePicker.

Done.

src/core/lib/transport/error_utils.h, line 43 at r5 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

At this point, I doubt we'll ever C++-ify grpc_error. Instead, we'll probably just replace it with absl::Status everywhere.

Ack

src/core/lib/transport/error_utils.cc, line 132 at r6 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

This should probably use the slice returned from grpc_error_get_status() rather than the whole error string.

If we do that, we will just get GRPC_ERROR_STR_GRPC_MESSAGE or GRPC_ERROR_STR_DESCRIPTION. Till we decide on how to use the absl::Status payload to capture all the contents of the grpc_error, I think we should use the entire error string. I'll add a TODO though.

markdroth

Just a couple of minor nits remaining!

Reviewed 5 of 6 files at r7, 1 of 1 files at r8.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @yashykt)

src/core/ext/filters/client_channel/subchannel.cc, line 1056 at r8 (raw file):

      gpr_log(GPR_INFO, "Connect failed: %s", grpc_error_string(error));
      error =
          grpc_error_set_int(GRPC_ERROR_REF(error), GRPC_ERROR_INT_GRPC_STATUS,

No need to do this here. This should be done by whatever handshaker generated the error.

src/core/ext/filters/client_channel/lb_policy/priority/priority.cc, line 656 at r6 (raw file):

Previously, yashykt (Yash Tibrewal) wrote…

I thought about it, but couldn't think of a reason to remove it. I would imagine that it would be better to have more information if we are printing this status somewhere else so that we can trace it.

I think we want these status strings to be easily human readable. These are messages for users, not for gRPC developers, so details about library internals (such as object addresses) are not relevant and should not be included.

That having been said, in this particular case, it probably doesn't matter right now, since this error message will never actually be returned to the application. However, please add a TODO assigned to me to clean this up as part of addressing #22883.

src/core/lib/transport/error_utils.cc, line 132 at r6 (raw file):

Previously, yashykt (Yash Tibrewal) wrote…

If we do that, we will just get GRPC_ERROR_STR_GRPC_MESSAGE or GRPC_ERROR_STR_DESCRIPTION. Till we decide on how to use the absl::Status payload to capture all the contents of the grpc_error, I think we should use the entire error string. I'll add a TODO though.

I think using GRPC_ERROR_STR_GRPC_MESSAGE or GRPC_ERROR_STR_DESCRIPTION is the right thing to do. That's exactly what gets used by the code in surface/call.cc when a grpc_error* is returned by the recv_trailing_metadata_ready callback.

We can later capture additional details in the absl::Status payload, but I don't think that will affect what we put in the message.

yashykt

Reviewable status: 39 of 42 files reviewed, 3 unresolved discussions (waiting on @markdroth)

src/core/ext/filters/client_channel/subchannel.cc, line 1056 at r8 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

No need to do this here. This should be done by whatever handshaker generated the error.

Hmm, not sure about that. I don't see either the tcp connector or the handshakers adding this status message. This seems like a good catchall place for handshakers in my opinion. Made the change though.

src/core/ext/filters/client_channel/lb_policy/priority/priority.cc, line 656 at r6 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

I think we want these status strings to be easily human readable. These are messages for users, not for gRPC developers, so details about library internals (such as object addresses) are not relevant and should not be included.

That having been said, in this particular case, it probably doesn't matter right now, since this error message will never actually be returned to the application. However, please add a TODO assigned to me to clean this up as part of addressing #22883.

Done. I think most likely this will be used for debugging purposes either by the user or by us, so the more information the better in general

src/core/lib/transport/error_utils.cc, line 132 at r6 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

I think using GRPC_ERROR_STR_GRPC_MESSAGE or GRPC_ERROR_STR_DESCRIPTION is the right thing to do. That's exactly what gets used by the code in surface/call.cc when a grpc_error* is returned by the recv_trailing_metadata_ready callback.

We can later capture additional details in the absl::Status payload, but I don't think that will affect what we put in the message.

Done

markdroth

Reviewed 3 of 3 files at r9.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @yashykt)

src/core/ext/filters/client_channel/subchannel.cc, line 1056 at r8 (raw file):

Previously, yashykt (Yash Tibrewal) wrote…

Hmm, not sure about that. I don't see either the tcp connector or the handshakers adding this status message. This seems like a good catchall place for handshakers in my opinion. Made the change though.

As with the LB policies, the principle here is that the place where the error is originally generated should set the status.

If you see places where this isn't happening, please feel free to fix that as part of this PR. :)

src/core/lib/transport/error_utils.cc, line 130 at r9 (raw file):

  // TODO(yashykt): This should be updated once we decide on how to use the
  // absl::Status payload to capture all the contents of grpc_error.
  grpc_slice message;

Don't we need to unref this slice when we're done with it?

yashykt

Reviewable status: 41 of 42 files reviewed, 1 unresolved discussion (waiting on @markdroth)

src/core/ext/filters/client_channel/subchannel.cc, line 1056 at r8 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

As with the LB policies, the principle here is that the place where the error is originally generated should set the status.

If you see places where this isn't happening, please feel free to fix that as part of this PR. :)

I'll prefer doing it separately.

src/core/lib/transport/error_utils.cc, line 130 at r9 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

Don't we need to unref this slice when we're done with it?

Oh! I thought this was C++ized already with RAII! Done

yashykt

Reviewable status: 41 of 42 files reviewed, 1 unresolved discussion (waiting on @markdroth)

src/core/lib/transport/error_utils.cc, line 130 at r9 (raw file):

Previously, yashykt (Yash Tibrewal) wrote…

Oh! I thought this was C++ized already with RAII! Done

Done.

yashykt

Reviewable status: 41 of 42 files reviewed, 1 unresolved discussion (waiting on @markdroth)

src/core/lib/transport/error_utils.cc, line 130 at r9 (raw file):

Previously, yashykt (Yash Tibrewal) wrote…

Done.

oh no! That is incorrect. It looks like the way this method is set up is that the callers need to explicitly take a ref if they need. Reverting

markdroth

Looks great!

Reviewed 1 of 3 files at r9.
Reviewable status: complete! all files reviewed, all discussions resolved

yashykt · 2020-07-23T02:29:34Z

Issues #22521

yashykt · 2020-07-23T02:33:02Z

Thanks for reviewing!

yashykt · 2020-07-23T02:33:48Z

Cherry-pick CL - cl/322675394
Alternatively patch CL - cl/322706004

Fix plugin initializers Previous code not threadsafe nor idiomatic Fix server_builder_plugin_test Bump version to v1.30.0-pre1 Regenerate projects Merge pull request grpc#23155 from srini100/v1.30.x bump v1.30.x branch to v1.30.0-pre1 Changes needed to enable TLS 1.3. Cleanup |ssl_bytes_remaining| per Yang's comments. Check unused bytes size is smaller than received bytes size, and fix Python format in _server_ssl_cert_config_test.py Revert "Check unused bytes size is smaller than received bytes size, and fix Python format in _server_ssl_cert_config_test.py" This reverts commit 7d0ae9c. Add check that unused bytes size is at most received bytes size. Add back source-only ruby packages for 1.30.x Fix memory leak, per Yang's comment. Initial working implementation Avoid collisions in cs files generated by Grpc.Tools Create generated directories Updated unit tests Reverted GetDepFilenameForProto Otherwise it creates conflicts with non-standard $(Protobuf_DepFilesPath) Improved folder generation Use same logic in csharp as in cpp Removed unused usings Reverted whitespace changes Readded InteropServices Needed #if NETCORE Bring closer to master Add support for Protobuf_ProtoRoot Fix false positive in grpc_is_epollexclusive_available on ESXi Improve third_party readme Fix up aio interop client Allow the user to specify grpclb config Merge remote-tracking branch 'upstream/master' into matt-tls13 Merge pull request grpc#23206 from ericgribkoff/dynamic_server_health Add gentle failover test Merge pull request grpc#23248 from ericgribkoff/use_grpc_health_check Use grpc health checks for PHP and Ruby xDS tests Run clang-format on is_epollexclusive_available.cc Merge pull request grpc#23250 from ericgribkoff/backport_gentle_failover Backport gentle failover Add TLS 1.2 testing. Merge remote-tracking branch 'upstream/master' into matt-tls13 Add #ifdef around use of SSL_ctx_min/max_proto_version Add include of tls1.h for OpenSSL. I is a reserved identifier Update links to grpc.io guides in header files Fix the grpcpp.h formatting. Merge pull request grpc#23186 from apolcyn/upmerge_source_only_to_1_30x Add back source-only ruby packages for 1.30.x Trust google-auth library will handle their dependency properly Install rsa==4.0 to avoid the egg cache race issue Add tls1.h header regardless of what SSL library is used. Bump version to 1.30.0 Regenerate projects Add #if defined(TLS1_3_VERSION). Merge pull request grpc#23264 from lidizheng/v1.30.x-patch Backport: Trust google-auth library will handle their dependency properly Merge pull request grpc#23266 from srini100/v1.30.x Bump to v1.30.0 Fix whitespaces Release dispatch queue on CFStreamHandle destroy Address David's comments. Move ServerPosix from ::grpc_impl to ::grpc Revert https://github.com/grpc/grpc/pull/18372/files Avoid attribute error in __del__ from _ChannelCallState Guard _ChannelCallState.__del__ from cygrpc being deallocated early Merge pull request grpc#23291 from lidizheng/v1.30.x-patch-2 Backport: Avoid attribute error in __del__ of _ChannelCallState Revert "Revert "Move ServerBuilder back to ::grpc from ::grpc_impl"" fix from sanitize.sh Merge pull request grpc#23303 from karthikravis/revert-23294-revert-23182-server-builder fix from sanitize.sh Merge branch 'master' into server-posix Move create_channel_posix from ::grpc_impl to ::grpc Revert grpc#18373 Merge remote-tracking branch 'upstream/master' into matt-tls13 Move create_channel and credentials from ::grpc_impl to ::grpc Reverts: grpc#18374 and grpc#18444 Credentials and create_channel are very closely intertwined, so it is easier to migrate them together. Make build and formatting fixes Add tests for unused bytes. Formatting fixes Implement gce_channel_credentials in core Fix formatting and build files Regenerate projects Initial implementation of gce_channel_creds Format Address Jiangtao's comments. Change name to compute_engine_channel_credentials Let the wrapped language create the composite credential Remove use of TLSv1_2_method. Expose gce tenancy as a function Regenerate projects Yapf :( Remove inline Stop double caching Add test Remove weird, seemingly pointless variable Clang format Main Makefile should require protobuf 3.12+ now Remove platform-specific assertion Buildifier Fix use-after-free in ruby call creds Lower log level to INFO from ERROR from keepalive watchdog firing Mark core API as experimental Use a gerund for a function name Simplify reference count juggling Clang format Merge pull request grpc#19756 from apolcyn/fix_ruby_md Fix auth plugin context use-after-free in ruby Make sure call creds user callbacks can't kill the ruby call credentials thread Update license date; fix format and typo Added missing deps_linkage Regenerated projects Fix typo Merge pull request grpc#23221 from chalin/chalin-grpc-io-link-fixes-200616 Update links to grpc.io guides in header files Merge pull request grpc#23335 from veblush/deps-linkage Added missing `deps_linkage` property Added missing deps_linkage Generated projects Fix gRPC-C++.podspec file Merge branch 'master' into revert-23294-revert-23182-server-builder Merge branch 'revert-23294-revert-23182-server-builder' of https://github.com/grpc/grpc into revert-23294-revert-23182-server-builder Merge pull request grpc#23324 from stanley-cheung/makefile-min-protobuf-version Main Makefile should require protobuf 3.12+ now Merge pull request grpc#23337 from CorralPeltzer/python-doc-typo Fix typo in docstring Merge branch 'master' into create-channel Fix credentials test. Address David's comments. Formatting fixes Suppress exceptions from the __del__ of channel object Merge pull request grpc#23304 from karthikravis/server-posix Move server posix from ::grpc_impl to ::grpc Merge pull request grpc#23305 from karthikravis/channel-posix Move create_channel_posix from ::grpc_impl to ::grpc Add back ciphersuites in ssl_utils.cc Merge pull request grpc#23345 from karthikravis/revert-23294-revert-23182-server-builder Revert "Revert "Move ServerBuilder back to ::grpc from ::grpc_impl"" Fix fetch oauth2 test Fix formatting issue Replaced grpc::string with std::string Merge pull request grpc#23350 from veblush/grpc-string2 Replaced grpc::string with std::string Removed grpc_artifact_protoc/Dockerfile Refactored AR in makefile Remove things_queued_ever counter from callback CQ Refactored AR in makefile Add abseil compiler options unify Grpc.Tools projects with other csproj projects Add missing include. Merge pull request grpc#23331 from yashykt/keepalivewatchdoginfolog Lower log level to INFO from ERROR from keepalive watchdog firing Fix connect deadline issue in settings_timeout_test Fix possible deadlock in RemoveExternalConnectivityWatcher Objc support PB runtime import override Fix possible deadlock in RemoveExternalConnectivityWatcher Merge branch 'master' into create-channel Merge pull request grpc#23354 from grpc/vjpai-patch-1 Remove things_queued_ever counter from callback CQ Merge pull request grpc#23364 from yashykt/settingstimeouttest Fix connect deadline issue in settings_timeout_test Removed GRPC_USE_ABSL Merge pull request grpc#23237 from grpc/jtattermusch-patch-1 Improve third_party readme (checklist for adding dependencies) Merge pull request grpc#22869 from Falco20019/grpc-fix-collisions Avoid collisions in cs files generated by Grpc.Tools update submodule boringssl-with-bazel with origin/master-with-bazel update boringssl dependency to master-with-bazel commit SHA Merge pull request grpc#23360 from markdroth/llvm_build_fix Add missing include. Merge pull request grpc#23165 from matthewstevenson88/matt-tls13 Enable TLS 1.3 in the C-core and all wrapped languages. Fix message_allocator_e2e_test Merge pull request grpc#23356 from veblush/ar-makefile Refactored AR in makefile Merge pull request grpc#23357 from veblush/absl-cppflags Add abseil compiler options Merge pull request grpc#23352 from veblush/protoc-docker Removed grpc_artifact_protoc/Dockerfile Merge pull request grpc#23298 from yulin-liang/prefix_import_path Objc support PB runtime import override Merge pull request grpc#23371 from veblush/del-GRPC_USE_ABSL Removed GRPC_USE_ABSL Merge pull request grpc#23376 from yang-g/allocator_test Fix message_allocator_e2e_test Added call to grpc::testing::TestEnvironment in tests Merge pull request grpc#23367 from yashykt/130fixws Fix possible deadlock in RemoveExternalConnectivityWatcher Merge pull request grpc#23378 from veblush/harden-tests Added call to grpc::testing::TestEnvironment in tests Added call to grpc::testing::TestEnvironment in more tests Merge branch 'master' into create-channel Merge pull request grpc#23379 from veblush/harden-tests2 Added call to grpc::testing::TestEnvironment in more tests Merge pull request grpc#23276 from grpc/cfstream-queue-release Release dispatch queue on CFStreamHandle destroy Add comment Merge pull request grpc#23333 from apolcyn/bad_md_drops_call_creds_thread Make sure call creds user callbacks can't kill the ruby call credentials thread Merge pull request grpc#23365 from yashykt/deadlockws Fix possible deadlock in RemoveExternalConnectivityWatcher Passed the repo manager to veblush Merge pull request grpc#23392 from veblush/to-veblush Passed the repo manager to veblush Increase size of a test to match internal guidance Pin isort to fix sanity test breakage New Matchers Implementation - All except for Regex (which will be submitted with RE2 import PR) Merge pull request grpc#22813 from mehrdada/fix-static-initialization-crap Fix channelz plugin initializer Merge pull request grpc#23395 from gnossen/lint_fix Pin isort to fix sanity test breakage Merge remote-tracking branch 'upstream/master' into matcher_new Merge pull request grpc#22951 from donnadionne/matcher_new New Matchers Add missing string include Get new core API design building Fix use-after-free in ruby call creds Add Python wrapper Pin isort to fix sanity test breakage Added TCP_USER_TIMEOUT auto-detection Merge pull request grpc#23393 from grpc/vjpai-patch-1 Increase size of a test to match internal guidance Merge pull request grpc#23400 from apolcyn/backport_sanity_check_fix Backport pylint sanity check fix to 1.30.x Bump 1.30.x branch to 1.30.1 Fix inproc transport bugs on client WritesDone or Read after status Don't drop message on WriteAndFinish with non-ok status regenerate projects generate boringssl prefix headers fix nits in third_party/README.md Increment podspec version regenerate projects Update README.md Add TLS 1.2 and 1.3 specific tests to h2_tls fixture. Merge pull request grpc#23380 from vjpai/inproc_fix Fix inproc transport bug on client WritesDone and server send message after status Merge pull request grpc#23409 from grpc/jtattermusch-patch-2 fix nits in third_party/README.md Merge pull request grpc#23386 from Kernald/fix-string-includes Add missing string include Merge pull request grpc#23408 from grpc/vjpai-patch-1 Don't drop message on WriteAndFinish with non-ok status Merge pull request grpc#23401 from veblush/tcp_user_timeout Added TCP_USER_TIMEOUT auto-detection Add gRPC-specific docker images for Ruby build Make pluing embed zlib Allows poller to bound to ephemeral loops in multiple threads Address comments Add type annotation in comments Merge pull request grpc#23399 from apolcyn/backport_auth_plugin_ruby_fix Backport fix for use-after-free in ruby auth plugin Replace most uses of gpr_asprintf() with absl calls. Merge pull request grpc#23387 from veblush/protoc-docker Make plugins embed zlib Merge pull request grpc#23377 from lidizheng/aio-multi-loop [Aio] Allows poller to bound to ephemeral loops in multiple threads Fix FailHijackedRecvMessage for generic async APIs Update grpclb with optional field "name" Add test for the new config field Address comments Add support for xDS regex matchers. Merge pull request grpc#23351 from lidizheng/silence-del Suppress exceptions from the __del__ of channel object Make sure that some ops don't start concurrently with Finish Merge pull request grpc#23405 from apolcyn/bump_version Bump 1.30.x branch to 1.30.1 Merge pull request grpc#22987 from donnadionne/new Add support for xDS regex matchers. Merge pull request grpc#23157 from markdroth/gpr_asprintf Replace most uses of gpr_asprintf() with absl calls. Store ref to the ExternalConnectivityWatcher in external_watchers_ map. Add comment Fix ruby 2.7 keyword arguments deprecation Similar to grpc#22915 Properly count messages sent Merge pull request grpc#23413 from matthewstevenson88/tls13-h2-tls-fixture Add TLS 1.2 and 1.3 specific tests to h2_tls fixture. Stop trying to handle SIGSEGV Make request path more easily visible to LB policies. Make warning more dire Merge pull request grpc#23416 from vjpai/ccet Make sure that some test ops don't start concurrently with Finish Merge pull request grpc#23421 from gnossen/test_runner_segfault Stop trying to handle SIGSEGV in Test Runner Merge pull request grpc#23258 from tweej/I_is_reserved I is a reserved identifier Merge pull request grpc#23417 from markdroth/lb_policy_path Make request path more easily visible to LB policies. Reviewer comments WIP EM-agnostic callback completion queue Fix TCP_USER_TIMEOUT definition Address comments Another comment Merge pull request grpc#23415 from yashykt/genericfailsend Fix FailHijackedRecvMessage for generic async APIs Merge pull request grpc#23425 from veblush/fix-TCP_USER_TIMEOUT Fix TCP_USER_TIMEOUT definition Fix data race in RpcWithEarlyFreeRequest by using atomics Fix data race in RpcWithEarlyFreeRequest by using atomics Merge pull request grpc#23358 from jtattermusch/cleanup_grpc_tools unify Grpc.Tools projects with other csproj projects Merge pull request grpc#23410 from jtattermusch/upgrade_boringssl_submodule Upgrade boringssl submodule to latest master-with-bazel add Grpc.Tools test for generating for .proto with the same name Merge pull request grpc#23427 from vjpai/message_allocator_race Fix data race in RpcWithEarlyFreeRequest by using atomics Merge pull request grpc#23361 from vjpai/em_agnostic_core_callback_cq EM-agnostic core callback CQ Remove unnecessary RPC Remove references to skipping callback API tests Reviewer comments Merge pull request grpc#22345 from muxi/grpclb-name-config Update grpclb configuration with field "service_name" Add sunny day core API path Merge pull request grpc#23419 from miyucy/patch-1 Fix ruby 2.7 keyword arguments deprecation Update the hint for install Python header to depend on version Stop using mutex for non-polling engine marker Make sure >3 Python version triggers a failure Merge pull request grpc#23418 from yashykt/dataraceexternalcancel Store ref to the ExternalConnectivityWatcher in external_watchers_ map. Merge pull request grpc#23430 from vjpai/remove_do_not_test Remove references to skipping callback API tests Remove unused variable (fixes builder error) Merge pull request grpc#23435 from vjpai/fix_iomgr_non_polling Fix-forward: Stop using mutex for non-polling engine marker Add test Clean up Reuse mdelem Merge remote-tracking branch 'origin/master' into python_google_default_creds Merge pull request grpc#23440 from grpc/vjpai-patch-1 Remove unused variable (fixes builder error) Merge pull request grpc#23375 from jtattermusch/csharp_proto_collision_test Add Grpc.Tools test for generating for .proto with the same name Added change_repo_manager.sh Changed a repo manager to karthikrs add comments for constants Fix initialization order Complete synchronously Accomodate PHP examples/README rework - Fix link to "Intro to gRPC" (formerly "Overview") - Don't list wrapped languages twice (once in intro paragraph, then in subdirectory list); move subdirectory links to the start of the page. - Sort subdirectory list - Link to grpc.io "Supported Languages" page for the remaining languages Merge pull request grpc#23446 from yang-g/init_order Fix initialization order Merge pull request grpc#23445 from HannahShiSFB/add-comments4constants PHP: add comments for constants make port server python3 compatible fix livelock in concurrent_connectivity_test on windows update bigquery partition expiration Update grpc_release_schedule.md Remove xds-experimental URI scheme. Merge pull request grpc#23444 from veblush/to-karthikrs Changed a repo manager to karthikrs Merge pull request grpc#22201 from chrisse74/memoryLeakBuildAndStart fix memory leak of grpc_resource_user_quota Merge pull request grpc#23437 from lidizheng/update-support-message Update the hint for install Python header to depend on version Merge branch 'master' into create-channel Fix build error Further reduce the spam log by doing nothing if GC has error Reorganize header Change signature Merge pull request grpc#23463 from markdroth/xds_experimental Remove xds-experimental URI scheme. Clean up Format Typo Merge remote-tracking branch 'origin/master' into python_google_default_creds Regenerate projects Merge pull request grpc#23447 from chalin/chalin-ex-readme-200710 examples/README rework Add XdsConfigSelector and populate call attribute containing the deadline. Fix secure test Fix formatting Merge pull request grpc#23467 from lidizheng/dealloc-message Further reduce the spam log by doing nothing if GC has error Update by review Move AuthMetaDataProcessor from ::grpc_impl to ::grpc Reverts: https://github.com/grpc/grpc/pull/18436/files Move GenericStub from ::grpc_impl to ::grpc Reverts: grpc#18434 Move LoadReportingServiceServerBuilderOption from ::grpc_impl to ::grpc Reverts: https://github.com/grpc/grpc/pull/18419/files Move ProtoServerReflectionPlugin from grpc_impl to grpc namespace Reverts: grpc#18600 Merge pull request grpc#23460 from markdroth/xds_config_selector Add XdsConfigSelector and populate call attribute containing the deadline. Fix tests Fix formatting Defer allocation of error object Add to Python documentation Support asyncio interop client Call out unexpected behavior adjust comment Merge pull request grpc#23462 from grpc/jtattermusch-patch-3 Update grpc_release_schedule.md Merge pull request grpc#23457 from jtattermusch/livelock_concurrent_connectivity_test fix potential livelock in global_subchannel_pool.cc bytes(string, encoding) is not supported in python2 results in error "TypeError: str() takes at most 1 argument (2 given)" Merge pull request grpc#23458 from jtattermusch/bigquery_partition_expirations Update bigquery partition expiration (to 365 days) Merge pull request grpc#23455 from jtattermusch/port_server_python3 Make port server python3 compatible Change repo manager to donnadionne This will be merged on 7/20/2020. Merge pull request grpc#23343 from veblush/v1.30.x-ruby [v1.30.x] Backport grpc#23335 (Added missing `deps_linkage` property) Updating C++ interop client and server to do path matching and header matching tests. Merge pull request grpc#23464 from donnadionne/new Updating C++ interop client and server Makefile: stop supporting make install remove rpath trick regenerate projects move routeguide c++ distribtest under cmake_pkgconfig fix C++ interop test build fix PHP interop tests build update PHP readme Merge pull request grpc#23412 from jtattermusch/makefile_remove_installation Simplify makefile: Get rid of "install" rules with pure make, recommend cmake and bazel instead. Include the target name in c-ares and native DNS summary error messages Adding Fake headers for header matching. Construct/destruct ExecCtx between init/shutdown only Add --verbose to dart pub get command Merge pull request grpc#23494 from donnadionne/ga Adding Fake headers for header matching. Concatenate duplicate header keys for matching. Merge pull request grpc#23495 from vjpai/fix_tls Construct/destruct ExecCtx between init/shutdown only Fixing call to absl::StrSplit to deal with empty string case. Merge pull request grpc#23499 from donnadionne/new Fixing call to absl::StrSplit to deal with empty string case. Merge pull request grpc#23496 from stanley-cheung/dart-debug Add --verbose to dart pub get command fix concurrent file read flake Avoid unused param warning Merge pull request grpc#23506 from grpc/vjpai-patch-2 Avoid unused param warning Merge pull request grpc#23493 from apolcyn/error_messages Include the target name in top-level DNS error messages Merge pull request grpc#23468 from karthikravis/auth_metadata Move AuthMetaDataProcessor from ::grpc_impl to ::grpc Merge pull request grpc#23469 from karthikravis/generic-stub Move GenericStub from ::grpc_impl to ::grpc Make more fixes for moving GenericStub from ::grpc_impl to ::grpc Merge pull request grpc#23512 from karthikravis/generic-stub Make more fixes for moving GenericStub from ::grpc_impl to ::grpc Merge pull request grpc#23470 from karthikravis/load-reporting Move LoadReportingServiceServerBuilderOption from ::grpc_impl to ::grpc Fix ruby protoc plugin when message is in another package Ruby protoc plugin fix: fix test breakage Merge pull request grpc#23471 from karthikravis/proto-server Move ProtoServerReflectionPlugin from grpc_impl to grpc namespace Merge pull request grpc#22589 from veblush/ruby-docker Ruby docker for gRPC Manylinux2010-based Ruby images Remove proto_server_reflection_plugin_impl.h from build_autogenerated.yaml Fixing unsafe std::move. Merge pull request grpc#23501 from stanley-cheung/fix-ruby-plugin Fix ruby protoc plugin when message is in another package Fix ruby protoc plugin when message is in another package Merge pull request grpc#23516 from karthikravis/fix-1 Remove proto_server_reflection_plugin_impl.h from build_autogenerated.yaml Merge pull request grpc#23517 from donnadionne/new Fixing unsafe std::move. Merge pull request grpc#23519 from stanley-cheung/ruby-protoc-plugin-fix-backport Backport grpc#23501: Fix ruby protoc plugin when message is in another package Bump version to 1.30.2 Avoid unused param warnings in c_ares directory Merge pull request grpc#23504 from jtattermusch/unflake_verify_peer_options Do not trigger grpc_load_file() flake in verify_peer_options_test Merge pull request grpc#23520 from stanley-cheung/bump-version-1-30-2 Bump version to 1.30.2 Merge pull request grpc#23505 from grpc/vjpai-patch-1 Remove some unused parameter names Merge pull request grpc#23491 from markdroth/xds_header_combining Concatenate duplicate header keys for matching. Revert "Merge pull request grpc#21941 from markdroth/xds_logging" This reverts commit 37d5d93, reversing changes made to 0e8190a. Fix PHP build by adding re2 dependencies Merge pull request grpc#23344 from veblush/ruby-docker-ml Ruby docker image based on manylinux2010 Upgrade BoringSSL to latest on master-with-bazel branch Uses sources.json uses sources.json update rules remove sha256 update sha256 update one more place Merge pull request grpc#23524 from markdroth/xds_logging_revert Revert "Merge pull request grpc#21941 from markdroth/xds_logging" Merge pull request grpc#23526 from stanley-cheung/fix-php-build Fix PHP build by adding re2 dependencies Fix php build from source Revert "Adding Fake headers for header matching." This reverts commit 8c013bf. Fix concatenated_value Fix formatting issue Merge pull request grpc#23538 from karthikravis/fix-xds-breakage Revert "Adding Fake headers for header matching." Update PHP artifact test generate api reference documents by doxygen Added grpc::testing::TestEnvironment Merge pull request grpc#23531 from stanley-cheung/fix-php-build-from-source Fix php build from source Updated comments C++ify core server Merge remote-tracking branch 'upstream/v1.30.x' into HEAD Merge pull request grpc#23236 from tpetkov-VMW/fix-grpc_is_epollexclusive_available Fix false positive in grpc_is_epollexclusive_available on ESXi Remove env var protection of new xds routing code. increase timeout for PHP distribtests Merge pull request grpc#23540 from stanley-cheung/php-artifact-test Update PHP artifact test Merge pull request grpc#23545 from veblush/test-shutdown Added call to grpc::testing::TestEnvironment in tests Merge pull request grpc#23476 from karthikravis/change-owner Change repo manager to donnadionne revert gen_build_yaml.py generate boringssl prefix headers Merge pull request grpc#23548 from jtattermusch/upmerge_1_30_x_to_master Upmerge 1.30.x branch into master Second regeneration update grpc-core podspec template regenerate project Merge pull request grpc#23537 from markdroth/xds_routing_env Remove env var protection of new xds routing code. Protect access Fix docstring Implement fake and ignored headers. Take care to ensure we use fake headers in testing so we don't conflict with headers used by census. Decouple checking tenancy from checking for metadata server Provide an objc-generator option to import system files in a local way Merge pull request grpc#23550 from jtattermusch/php_distrib_15min increase timeout for PHP distribtests Merge pull request grpc#23539 from donnadionne/fix Adding Fake headers and ignored headers for header matching Change xDS ConfigSelector not to pass the timeout header value to the LB policy. Merge pull request grpc#23475 from yulin-liang/local_import_prefix Objc support grpc files import override Initialize the grpc_local_import variable to false. Include re2 in source wheel && solve the missing --all complain Stop checking g_is_on_gce in security connector Merge pull request grpc#23555 from yulin-liang/local_import_prefix Initialize the grpc_local_import variable to false. Log the peer address of grpc_cli CallMethod RPCs to stderr Explicitly fake status if cancelling stream Drop min thread count of threads doing Next to 1 Merge pull request grpc#23255 from HannahShiSFB/api-doxygen-step2 PHP: generate api reference documents by doxygen Merge pull request grpc#23534 from markdroth/xds_fake_header_revert Change xDS ConfigSelector not to pass the timeout header value to the LB policy. stop stripping .hpp files from python source packages Merge pull request grpc#23562 from yang-g/one_core Drop min thread count of threads doing Next to 1 Merge pull request grpc#23528 from emkornfield/upgrade_boring Upgrade BoringSSL to latest on master-with-bazel branch Merge pull request grpc#23552 from lidizheng/re2-source-wheel Include re2 in source wheel && solve the missing --all complain Merge pull request grpc#23565 from jtattermusch/python_dev_stop_excluding_hpp Stop stripping .hpp files from python source packages avoid destroy channel more than once Don't check tenancy if credentials specified Use RAII lock guard Support tuple and aio.Metadata interaction Make pytype happy Only >=3.7 allows annotate oneself as return type Metadata value has to be string or bytes Remove MAX_EPOLL_EVENTS_HANDLED_EACH_POLL_CALL to ensure timely processing of events Remove unused variable Add logs to a flakey ruby test that can help to diagnose issue Add comment Merge pull request grpc#23556 from lidizheng/fix-gapic-tests [Aio] Support tuple and aio.Metadata interaction Merge pull request grpc#23535 from apolcyn/repro_epollex_issue_minimal Remove MAX_EPOLL_EVENTS_HANDLED_EACH_POLL_CALL to ensure timely processing of events Output grpc-dotnet interop apps to specific directories Merge pull request grpc#23570 from apolcyn/fix_ruby_flake Add logs to a flakey ruby test that can help to diagnose issue Also fake status in grpc_chttp2_mark_stream_closed if already closed but there is an error Make ABSL header block consistent with other code. Merge pull request grpc#23558 from yashykt/cancelstreamdeadline Explicitly fake the status for when we cancel the streams Fix Mac and Windows Merge pull request grpc#22757 from vjpai/core_server_cxx C++ify grpc_server Fix a typo Convert multi-line comments to single line comments in chttp2_transport.cc Merge branch 'master' into chttp2transportcomment add g-stands-for for the next release update version to 1.32.0-dev Merge pull request grpc#23523 from JamesNK/jamesnk/interoptests-remove-version Output grpc-dotnet interop apps to specific directories regenerate projects Merge pull request grpc#23203 from gnossen/python_google_default_creds Implement compute_engine_channel_credentials in Python Clang format Plumb absl::Status through connectivity state notifiers Merge pull request grpc#23480 from yashykt/abslstatusconn Plumb absl::Status through connectivity state notifiers Merge branch 'master' into chttp2transportcomment Flag protect new logs Merge pull request grpc#23576 from jtattermusch/bump_master_1_32 update master version to 1.32.0-dev Merge branch 'master' into create-channel Consistency check core_version and version Fixes to match shellcheck Merge pull request grpc#23578 from vjpai/core_check Add a consistency check on version and core_version Merge pull request grpc#23571 from vjpai/typos Typo fix xDS v3 support Merge pull request grpc#23557 from apolcyn/display_peer_address Log the peer address of grpc_cli CallMethod RPCs to stderr fixed merge issues Merge pull request grpc#23281 from markdroth/xds_v3 xDS v3 support Make fixes to cronet_credentials.h Fixing Ruby 1.31.0.pre1 compilation failure with source-only package missing re2 from build_handwritten.yaml and generated grpc.gemspec Merge pull request grpc#23599 from donnadionne/ruby Fixing Ruby compilation failure with source-only package Merge pull request grpc#23308 from karthikravis/create-channel Move create_channel and credentials from ::grpc_impl to ::grpc Disable TLS 1.3 in the C-core. Revert "Move create_channel and credentials from ::grpc_impl to ::grpc" Disable TLS 1.3-specific unit tests in ssl_transport_security_test.cc. Fix number of TLS version tests to run. Merge pull request grpc#23609 from grpc/revert-23308-create-channel Revert "Move create_channel and credentials from ::grpc_impl to ::grpc" Merge pull request grpc#23567 from HannahShiSFB/23477 PHP: avoid destroy channel more than once Merge pull request grpc#23608 from matthewstevenson88/matt-disable-tls13 Disable TLS 1.3 in the C-core. Fix BUILD file for import. Member variable should not be a reference; causing crash when used in destructor. Merge pull request grpc#23575 from yashykt/chttp2transportcomment Comment formatting in chttp2_transport Merge pull request grpc#23610 from donnadionne/crash Member variable should not be a reference; causing crash when used in destructor. Merge pull request grpc#23612 from markdroth/xds_v3_build_fix Fix BUILD file for import. Passing repo manager to markdroth Merge pull request grpc#23624 from donnadionne/rm Passing repo manager to markdroth resolved merge conflicts with envoy v3 api

markdroth · 2021-10-11T14:32:54Z

This PR was a necessary but not sufficient part of solving that more general problem. The more general issue is tracked in #22883.

yashykt force-pushed the abslstatusconn branch 3 times, most recently from cd51a45 to b4f6b9c Compare July 16, 2020 13:18

yashykt added the release notes: no Indicates if PR should not be in release notes label Jul 16, 2020

yashykt changed the title ~~Abslstatusconn~~ Plumb absl::Status through connectivity state notifiers Jul 16, 2020

yashykt force-pushed the abslstatusconn branch from db896c5 to 419d5d7 Compare July 16, 2020 13:47

yashykt marked this pull request as ready for review July 16, 2020 13:49

yashykt requested a review from markdroth as a code owner July 16, 2020 13:49

markdroth requested changes Jul 17, 2020

View reviewed changes

yashykt commented Jul 18, 2020

View reviewed changes

yashykt force-pushed the abslstatusconn branch from 2205ef7 to 5f0f4ed Compare July 18, 2020 05:05

yashykt mentioned this pull request Jul 18, 2020

Keepalive Throttling #23313

Merged

markdroth requested changes Jul 20, 2020

View reviewed changes

yashykt commented Jul 20, 2020

View reviewed changes

markdroth requested changes Jul 21, 2020

View reviewed changes

yashykt commented Jul 21, 2020

View reviewed changes

markdroth requested changes Jul 22, 2020

View reviewed changes

yashykt commented Jul 22, 2020

View reviewed changes

markdroth requested changes Jul 22, 2020

View reviewed changes

yashykt commented Jul 22, 2020

View reviewed changes

yashykt force-pushed the abslstatusconn branch from 8a5fc80 to bc5705e Compare July 22, 2020 21:02

markdroth approved these changes Jul 22, 2020

View reviewed changes

Plumb absl::Status through connectivity state notifiers

2c5291f

yashykt force-pushed the abslstatusconn branch from bc5705e to 2c5291f Compare July 22, 2020 22:57

yashykt merged commit be58863 into grpc:master Jul 23, 2020

markdroth mentioned this pull request Jul 28, 2020

plumb subchannel connection failure status up to LB policy #22885

Closed

markdroth mentioned this pull request May 13, 2021

P5: Require a gRFC for large scale changes to gRPC core grpc/proposal#233

Closed

yashykt deleted the abslstatusconn branch May 18, 2023 20:16

Conversation

yashykt commented Jul 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

markdroth left a comment

Choose a reason for hiding this comment

Uh oh!

yashykt left a comment

Choose a reason for hiding this comment

Uh oh!

yashykt left a comment

Choose a reason for hiding this comment

Uh oh!

markdroth left a comment

Choose a reason for hiding this comment

Uh oh!

yashykt left a comment

Choose a reason for hiding this comment

Uh oh!

markdroth left a comment

Choose a reason for hiding this comment

Uh oh!

yashykt left a comment

Choose a reason for hiding this comment

Uh oh!

markdroth left a comment

Choose a reason for hiding this comment

Uh oh!

yashykt left a comment

Choose a reason for hiding this comment

Uh oh!

markdroth left a comment

Choose a reason for hiding this comment

Uh oh!

yashykt left a comment

Choose a reason for hiding this comment

Uh oh!

yashykt left a comment

Choose a reason for hiding this comment

Uh oh!

yashykt left a comment

Choose a reason for hiding this comment

Uh oh!

markdroth left a comment

Choose a reason for hiding this comment

Uh oh!

yashykt commented Jul 23, 2020

Uh oh!

yashykt commented Jul 23, 2020

Uh oh!

yashykt commented Jul 23, 2020

Uh oh!

markdroth commented Oct 11, 2021 via email

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

yashykt commented Jul 14, 2020 •

edited

Loading