Commit Graph

5569 Commits

Author SHA1 Message Date
apolcyn 2a4b7f25e6
testing: add a fake TCP and UDP server to test/core/util (extracted from #27883) (#28332)
* add a fake TCP and UDP server to test/core/util
2021-12-14 08:50:17 -08:00
Jan Tattermusch 32770542b2
Cleanup of ObjC test scripts (#28333)
* cleanup cfstream tests scripts

* add xcodebuild_filter_output script

* cleanup scripts for ObjC CoreTests

* clean one more build_tests.sh file

* cleanup more scripts

* cleanup ios cpp scripts

* remove last copy of verbose_time

* revert readlink change
2021-12-14 09:22:04 +01:00
Ashitha Santhosh 4fd524cd17
Fix presence match in authorization. (#28269)
* Fix presence match in authorization.

* Remove header

* Add test

* fix regex to include whitespace characters
2021-12-13 14:53:05 -08:00
Craig Tiller 0deb64d1f6
Move content-type, colon prefixed metadata to new system (#28204)
* Eliminate most of grpc_message metadata handling

* Eliminate most of host metadata handling

* Remove more callouts without fixing code

* fiiixes

* typo

* Automated change: Fix sanity tests

* try-shrink

* Automated change: Fix sanity tests

* size tweaks

* less tricks

* deunique

* commonize

* commonize

* Automated change: Fix sanity tests

* size tuning, fixes

* Automated change: Fix sanity tests

* fix

* size tuning, fixes

* remove constexpr

* fix

* reuse code

* fix

* tweak code

* more tweaks

* tell no lies

* fixes

* fixes

* Automated change: Fix sanity tests

* fix

* fix

* fix

* fix

* fix?

* fix binder

* fix

* fix

* fixes

* Automated change: Fix sanity tests

* fix

* initial refactoring

* optimize status encoding

* Automated change: Fix sanity tests

* Automated change: Fix sanity tests

* content-type

* Automated change: Fix sanity tests

* clang-format

* fix

* Move colon prefixed metadata

* Automated change: Fix sanity tests

* Automated change: Fix sanity tests

* try to fix windows failure

* try and scale sizes better

* ambiguity fix?

* wip metadatavalueasslice

* Fix status code for resource exhaustion

* Revert "Revert "Move a bunch of slice typed metadata to new system (#28107)" (#28208)"

This reverts commit 7717587063.

* fix test

* Automated change: Fix sanity tests

* Automated change: Fix sanity tests

* slice helper

* x

* noinline

* try and scale sizes better

* Automated change: Fix sanity tests

* fixes

* fix

* fix

* fixes

* fix build

* fix overflow

* progress

* Automated change: Fix sanity tests

* fix

* fix

* fix

* Automated change: Fix sanity tests

* fix

* fix

* fix

* compressor for path/authority

* Automated change: Fix sanity tests

* legalize

* status-enc

* fmt

* fix

* fix

* fix

* fix

* fix/opt

* fix

* fix

* fix

* fix

* Automated change: Fix sanity tests

* remove arg

* review feedback

* fix

* Small improvement in memory usage and performance

* Automated change: Fix sanity tests

* fix crash in alts

* review feedback

* fixes

* fixes

* fixes

* Automated change: Fix sanity tests

* speedup

* fix

* fix

* add comment

* add comment

Co-authored-by: ctiller <ctiller@users.noreply.github.com>
2021-12-13 12:11:45 -08:00
Mark D. Roth f76f2653c3
Second attempt: XdsClient: remove resource-type-specific methods from XdsClient API (#28305)
* Revert "Revert "XdsClient: remove resource-type-specific methods from XdsClient API (#28231)" (#28301)"

This reverts commit 3e779b68fe.

* remove global resource type registry
2021-12-08 09:24:33 -08:00
Mark D. Roth 3e779b68fe
Revert "XdsClient: remove resource-type-specific methods from XdsClient API (#28231)" (#28301)
This reverts commit 747a5d8c24.
2021-12-07 15:46:10 -08:00
Mark D. Roth 747a5d8c24
XdsClient: remove resource-type-specific methods from XdsClient API (#28231)
* WIP

* introduce XdsResourceType API and change Listener parsing to use it

* converted RouteConfig parsing

* convert cluster and endpoint parsing

* cleanup

* clang-format

* attempt to work around compiler problems

* move XdsResourceType to its own file, and move endpoint code out of XdsApi

* move cluster parsing to its own file

* move route config parsing to its own file

* move listener parsing to its own file

* clang-format

* minor cleanup

* plumbed XdsResourceType throughout XdsClient

* a bit of cleanup

* more cleanup

* construct full resource names before calling XdsApi::CreateAdsRequest()

* remove some unneeded code

* clean up includes and have XdsResourceType initialize the upb symtab

* more cleanup of unnecessary code

* more cleanup

* update comment

* clang-format

* add missing virtual dtor

* fix build

* remove resource-type-specific methods from XdsClient API

* have each resource type register itself upon instantiation

* remove comment

* add missing virtual dtor

* clang-format
2021-12-07 11:00:08 -08:00
Mark D. Roth ac70281e74
improve RPC status messages when name resolution fails (#28091)
* improve RPC status messages when name resolution fails

* fix tests

* clang-format

* fix rls_end2end_test

* fix build

* fix service_config_end2end_test

* fix too_many_pings_test

* fix tests

* fix client_channel to propagate resolution_note to LB policy

* improve comment
2021-12-07 09:05:38 -08:00
Craig Tiller b95ed96b96
Revert "Revert "Move arena into resource quota (#28008)" (#28292)" (#28293)
This reverts commit 91edf92ce0.
2021-12-06 21:41:06 -08:00
Craig Tiller 91edf92ce0
Revert "Move arena into resource quota (#28008)" (#28292)
This reverts commit ad424a2ebf.
2021-12-06 12:59:28 -08:00
Craig Tiller ad424a2ebf
Move arena into resource quota (#28008)
* new resource quota integration

* Automated change: Fix sanity tests

* fix

* fix

* fixes

* fixes

* fixes

* Automated change: Fix sanity tests

* fixes

* fixes

* Automated change: Fix sanity tests

* fixes

* fix

* fixes

* windows-fix

* fixes

* fixes

* fix

* fix-asan

* banned

* banned

* fixes

* clang-tidy-fix

* Automated change: Fix sanity tests

* fix-cronet

* review feedback

* review feedback

* Automated change: Fix sanity tests

* fixes

* bug fix

* fixes

* compile fix

* exclude megabyte size payloads from 1byte tests

* windows fix

* start moving ios

* keep moving windows

* Get windows compilation working.

* Automated change: Fix sanity tests

* better

* fixes

* remove slice buffer from memory_allocator.h

* Revert "remove slice buffer from memory_allocator.h"

This reverts commit 234a63b6e3.

* ugh

* #fixtests

* pthread tls fixes

* Automated change: Fix sanity tests

* fixfixfix

* xxx

* add reset

* review feedback

* fix

* fix

* fixes

* fix

* mac progress

* cpp-impl-of

* rename ptr

* Automated change: Fix sanity tests

* memory-owner-is-a-memory-allocator

* fixes

* fix

* fix from prod

* fix

* Fix issue leading to bad pointers being returned on Windows.

* Automated change: Fix sanity tests

* fix multislice bug

* argh

* hyrums law fixes

* hyrums law fixes

* clang-format

* hyrums law fixes

* Move arena into resource quota

* Remove redundant constant

* fixup the tests

* Automated change: Fix sanity tests

* fiiixes

* Automated change: Fix sanity tests

* fixes

* Automated change: Fix sanity tests

* fixes

* fix

* fix

* Automated change: Fix sanity tests

* fixes

Co-authored-by: ctiller <ctiller@users.noreply.github.com>
2021-12-06 08:43:56 -08:00
Mark D. Roth 81a4ea444e
xds: remove legacy EDS-only workflow (#28274)
* xds: remove legacy EDS-only workflow

* remove unused constants

* remove unused data member
2021-12-06 08:00:15 -08:00
Craig Tiller e21505858f
Move some integer based metadata to the new system (#28198)
* Eliminate most of grpc_message metadata handling

* Eliminate most of host metadata handling

* Remove more callouts without fixing code

* fiiixes

* typo

* Automated change: Fix sanity tests

* try-shrink

* Automated change: Fix sanity tests

* size tweaks

* less tricks

* deunique

* commonize

* commonize

* Automated change: Fix sanity tests

* size tuning, fixes

* Automated change: Fix sanity tests

* fix

* size tuning, fixes

* remove constexpr

* fix

* reuse code

* fix

* tweak code

* more tweaks

* tell no lies

* fixes

* fixes

* Automated change: Fix sanity tests

* fix

* fix

* fix

* fix

* fix?

* fix binder

* fix

* fix

* fixes

* Automated change: Fix sanity tests

* fix

* initial refactoring

* optimize status encoding

* Automated change: Fix sanity tests

* Automated change: Fix sanity tests

* fix

* try to fix windows failure

* try and scale sizes better

* ambiguity fix?

* wip metadatavalueasslice

* Fix status code for resource exhaustion

* Revert "Revert "Move a bunch of slice typed metadata to new system (#28107)" (#28208)"

This reverts commit 7717587063.

* fix test

* Automated change: Fix sanity tests

* slice helper

* x

* noinline

* try and scale sizes better

* Automated change: Fix sanity tests

* fixes

* fix

* fix

* fix build

* fix overflow

* fix

* fix

* fix

* fix

* review feedback

Co-authored-by: ctiller <ctiller@users.noreply.github.com>
2021-12-02 22:02:56 -08:00
Yash Tibrewal 3689072979
xDS: Add graceful shutdown for old connections on listener resource update (#28154)
* xDS: Add graceful shutdown for old connections on listener resource update

* Add TODOs for review

* Reviewer comments

* Reviewer comments

* Fix merge

* Fix comment

* s/GRPC_ARG_DRAIN_GRACE_TIME_MS/GRPC_ARG_SERVER_CONFIG_CHANGE_DRAIN_GRACE_TIME_MS
2021-12-02 15:42:47 -08:00
Esun Kim 260145c517
Updated backoff to use absl::Random (#27193)
* Replace in-house rand with absl::Random for backoff (#26463)

* Replace in-house rand with absl::Random for backoff

* Run sanity

* Added bscrypt dependency

* Removed ABSL_RANDOM_HWAES_FLAGS

* Remove GCC_WARN_64_TO_32_BIT_CONVERSION = YES

* GCC_WARN_64_TO_32_BIT_CONVERSION to podspec install
2021-12-02 15:22:44 -08:00
apolcyn c6b9483a30
add test for c-ares resolver error message propogation (#28252) 2021-12-02 09:14:06 -08:00
Mark D. Roth 9722651156
xds_end2end_test: remove num_balancers from test suite and remove dependence on fake resolver for xDS channel (#28245)
* xds_end2end_test: remove num_balancers from test suite

* fix clang-tidy
2021-12-01 16:28:15 -08:00
Craig Tiller 2eadef9a82
Remove bm_metadata (#28219) 2021-11-29 13:40:26 -08:00
Craig Tiller 220cf641fa
Reland slice metadata conversion (#28210)
* Fix status code for resource exhaustion

* Revert "Revert "Move a bunch of slice typed metadata to new system (#28107)" (#28208)"

This reverts commit 7717587063.

* fix test
2021-11-29 13:38:40 -08:00
Craig Tiller 7717587063
Revert "Move a bunch of slice typed metadata to new system (#28107)" (#28208)
This reverts commit 4d432aefc1.
2021-11-24 15:41:36 -08:00
Craig Tiller 4d432aefc1
Move a bunch of slice typed metadata to new system (#28107)
* Eliminate most of grpc_message metadata handling

* Eliminate most of host metadata handling

* Remove more callouts without fixing code

* fiiixes

* typo

* Automated change: Fix sanity tests

* try-shrink

* Automated change: Fix sanity tests

* size tweaks

* less tricks

* deunique

* commonize

* commonize

* Automated change: Fix sanity tests

* size tuning, fixes

* Automated change: Fix sanity tests

* fix

* size tuning, fixes

* remove constexpr

* fix

* reuse code

* fix

* tweak code

* more tweaks

* tell no lies

* fixes

* fixes

* Automated change: Fix sanity tests

* fix

* fix

* fix

* fix

* fix?

* fix binder

* fix

* fix

* fixes

* Automated change: Fix sanity tests

* fix

Co-authored-by: ctiller <ctiller@users.noreply.github.com>
2021-11-24 11:42:37 -08:00
Jan Tattermusch ce74e092e6
add --log_metadata_and_status feature to interop_client (#28021) 2021-11-23 15:32:13 -08:00
Craig Tiller 672f31c6ac
Channel args preconditioning (#28132)
* Channel args preconditioning

* docs

* fixes

* Automated change: Fix sanity tests

* fix

* fix this again after merge error

Co-authored-by: ctiller <ctiller@users.noreply.github.com>
2021-11-22 15:24:39 -08:00
Mark D. Roth 0bdb4d650b
Revert "Revert "use CppImplOf for grpc_server (#28112)" (#28130)" (#28144)
This reverts commit eec0ca98c1.
2021-11-19 09:15:04 -08:00
Yash Tibrewal 25446c468a
Support RDS updates on the server (#27851)
* Port changes from #27388

* Reviewer comments

* Fix resource timeout issue

* Cleanup

* Fix clang-tidy

* Revert benchmark

* Restructure

* clang-tidy

* Automated change: Fix sanity tests

* Partial commit

* Reviewer comments

* Fixes

* Reviewer comments

* Reviewer comments

* Reviewer comments

* Reviewer comments

* clang-format

* Fix FaultInjection tests

* clang-tidy

Co-authored-by: yashykt <yashykt@users.noreply.github.com>
2021-11-18 16:10:31 -08:00
Esun Kim 11f440775b
Fix xds_end2end_test dyld (#28133) 2021-11-18 16:08:41 -08:00
Mark D. Roth eec0ca98c1
Revert "use CppImplOf for grpc_server (#28112)" (#28130)
This reverts commit 2ea8e50c3a.
2021-11-18 14:50:39 -08:00
Mark D. Roth 1050eedb87
enable clang-tidy readability-static-definition-in-anonymous-namespace check (#28033) 2021-11-17 07:48:43 -08:00
Mark D. Roth 2ea8e50c3a
use CppImplOf for grpc_server (#28112)
* use CppImplOf for grpc_server

* fix build

* fix sanity
2021-11-17 07:46:53 -08:00
Mark D. Roth fff84402ac
Second attempt: XdsClient: fix resource timeout behavior (#28088)
* Revert "Revert "XdsClient: fix resource timeout behavior (#27860)" (#28032)"

This reverts commit 817eed0928.

* use the right status code enum
2021-11-15 14:53:15 -08:00
Craig Tiller a629c9a03e
Reland resource quota work (#28017)
* Check if memory owner available prior to polling it

The transport may drop the memory owner during its destruction sequence

* tcp_fix

* Revert "Revert "New resource quota integration (#27643)" (#28014)"

This reverts commit 0ea2c37263.

* clang-format

* fix-path

* fix
2021-11-15 08:23:51 -08:00
Craig Tiller 817eed0928
Revert "XdsClient: fix resource timeout behavior (#27860)" (#28032)
This reverts commit 7fdb40d8b8.
2021-11-12 19:58:16 -08:00
Mark D. Roth 7fdb40d8b8
XdsClient: fix resource timeout behavior (#27860)
* XdsClient: fix resource timeout behavior

* fix clang-tidy

* more clang-tidy fixes

* yet more clang-tidy
2021-11-12 12:17:45 -08:00
Craig Tiller 0ea2c37263
Revert "New resource quota integration (#27643)" (#28014)
This reverts commit 39f0877d25.
2021-11-11 14:26:19 -08:00
Craig Tiller 39f0877d25
New resource quota integration (#27643)
* new resource quota integration

* Automated change: Fix sanity tests

* fix

* fix

* fixes

* fixes

* fixes

* Automated change: Fix sanity tests

* fixes

* fixes

* Automated change: Fix sanity tests

* fixes

* fix

* fixes

* windows-fix

* fixes

* fixes

* fix

* fix-asan

* banned

* banned

* fixes

* clang-tidy-fix

* Automated change: Fix sanity tests

* fix-cronet

* review feedback

* review feedback

* Automated change: Fix sanity tests

* fixes

* bug fix

* fixes

* compile fix

* exclude megabyte size payloads from 1byte tests

* windows fix

* start moving ios

* keep moving windows

* Get windows compilation working.

* Automated change: Fix sanity tests

* better

* fixes

* remove slice buffer from memory_allocator.h

* Revert "remove slice buffer from memory_allocator.h"

This reverts commit 234a63b6e3.

* ugh

* #fixtests

* pthread tls fixes

* Automated change: Fix sanity tests

* fixfixfix

* xxx

* add reset

* review feedback

* fix

* fix

* fixes

* fix

* mac progress

* cpp-impl-of

* rename ptr

* Automated change: Fix sanity tests

* memory-owner-is-a-memory-allocator

* fixes

* fix

* fix from prod

* fix

* Fix issue leading to bad pointers being returned on Windows.

* Automated change: Fix sanity tests

* fix multislice bug

* argh

* hyrums law fixes

* hyrums law fixes

* clang-format

* hyrums law fixes

Co-authored-by: ctiller <ctiller@users.noreply.github.com>
2021-11-10 22:53:04 -08:00
Craig Tiller 75a54f55e3
Remove trickle benchmarks (#28000)
* Remove trickle benchmarks

* Automated change: Fix sanity tests

Co-authored-by: ctiller <ctiller@users.noreply.github.com>
2021-11-10 18:21:13 -08:00
ZhenLian 2e14f6fa70
Support Custom Post-handshake Verification in TlsCredentials (#25631)
* custom verification refactoring - post-handshake verification
2021-11-10 13:45:52 -08:00
Yash Tibrewal 00e8a2d085
xds_end2end_test: Only start backends when needed (#27911)
* xds_end2end_test: Fix flakiness on WaitForLdsNack

* xds_end2end_test: Only start the server when we want

* Revert WaitForNack changes

* Fixes

* Fix CsdsShortAdsTimeoutTest

* Fix sanity
2021-11-04 14:35:25 -07:00
Richard Belleville 468768865b
Make Buildifier Sanity Test Strict (#27807)
* Fix all lint errors in repo.

* Use strict buildifier by default

* Whoops. That file does not exist

* Attempt fix to buildifier invocation

* Add missing copyright
2021-11-03 14:57:04 -07:00
Mark D. Roth e251ee9ffb
disable authorization_policy_provider_test on iOS (#27927) 2021-11-03 11:55:32 -07:00
Jan Tattermusch 6001bc636b
Stop running some more esoteric C++ benchmark scenarios continuously (#27872)
* stop running some more esoteric C++ benchamk scenarios continuously

* regenerate qps scenarios for bazel unit tests
2021-11-02 11:27:00 +01:00
Jan Tattermusch cf99a32109
set threads_per_cq=2 for C++ unconstrained scenarios for best throughput and lower variance (#27870) 2021-11-02 06:57:53 +01:00
Mark D. Roth 3bde8305cc
Second attempt: Don't build RLS on mobile (#27878)
* Revert "revert changes to not build RLS on mobile (#27875)"

This reverts commit 73ad459c51.

* new approach using multiple select() calls
2021-11-01 10:26:01 -07:00
Hannah Shi 6f4f920398
switch to official protobuf podspec (#27830) 2021-10-29 15:58:34 -07:00
Mark D. Roth 73ad459c51
revert changes to not build RLS on mobile (#27875)
* Revert "fix mobile builds to properly exclude xDS (#27855)"

This reverts commit 64d026e234.

* Revert "don't build RLS on mobile (#27838)"

This reverts commit 2a69c525b8.
2021-10-29 12:00:00 -07:00
Mark D. Roth 08c7b254bd
provide more slack in RLS end2end tests (#27842) 2021-10-27 15:49:54 -07:00
Mark D. Roth 2a69c525b8
don't build RLS on mobile (#27838)
* don't build RLS on mobile

* buildifier

* fix build rules to avoid "Illegal ambiguous match" bazel error
2021-10-27 15:46:00 -07:00
Vy Nguyen 70b26a1c7c
Upgrade benchmark to 1.6.0 and remove previous hacks. (#27778)
* Upgrade benchmark to 1.6.0 and remove hacks.

Details:
 - GRPC currently uses an old version of benchmark (from Sept 2020). It should probably upgrade because downstream, in google3, everyone is already using 1.6.0)
 - Removed the hack added in PR/27629 to allow benchmarks in GRPC to continue to work with both pre-1.6.0 and 1.6.0 benchmarks.
   (This was needed to allow importing benchmarks 1.6.0 into google3 without breaking GRPC)

* fix typo

* update third_party/benchmark and check_submodules.sh

* Upmerge from v1.41.x (#27821)

* Bump version to v1.41.0-pre1 (#27371)

* Bump version to v1.41.0-pre1

* Regenerate projects

* [Backport #27373] add testing_version flag (#27385)

* Bump version to v1.41.0-pre2 (#27390)

* Bump version to v1.41.0-pre2

* Regenerate projects

* Core 19: bump core version from 18.0.0 to 19.0.0 (#27394)

* Bump core version to 19.0.0

* Regenerate projects

* fix use-after-free metadata corruption in C# when receiving response headers for streaming response calls (#27398)

* Final release: bump up version to 1.41.0 (#27476)

* Bump version to 1.41.0

* Regenerate projects

* xds_k8s_test: increase timeout to 3 hours due to recent timeout failure (#27580)

* Revert "xds_k8s_test: increase timeout to 3 hours due to recent timeout failure (#27580)" (#27590)

This reverts commit da0c7d680f.

* Update root pem certs (backport of #27539) (#27619)

* Update boringssl to the latest (#27606) (#27625)

* Change boringssl branch name

* update submodule boringssl-with-bazel with origin/main-with-bazel

* update boringssl dependency to main-with-bazel commit SHA

* regenerate files

* Increment podspec version

* generate boringssl prefix headers

* Bumping up version to v1.41.1 (#27699)

* Bump version to v1.41.1

* Regenerate projects

* [Backport][v1.41.x] xds-k8s tests: Use test driver from master branch (#27695)

Backports sourcing the test driver install script from master.

This is a backport of #27389, #27462 and #27658:


* Add missing quatation marks.

These were missed when  creating the Python virtual env.

* xds-k8s tests: Use test driver from master branch (#27462)

Instead of directly sourcing the test driver provisioning script from the same branch, the script is downloaded (with curl) and sourced from the master branch.

This allows changes made to the test driver to be reflected in all future release branches. A separate PR will backport this change to existing release branches.

All cluster definitions are also moved to the install script, allowing any cluster changes to be done in one place in the master branch.

* xds_k8s tests: Fix xlang install script sourcing. (#27658)

This change sources the test driver install script correctly for the xlang tests.
This fixes a mistake in #27462 where this was missed.

* Fix Python Interop (#27620) (#27703)

* WIP. Attempt to fix interop

* Yapf

* Switch Python xDS Example Server to Listen on IPV4 Only (#27679)

* Switch to IPV4

* Update to all hosts

* Fix rvm ruby install failure (#27769)

Co-authored-by: donnadionne <donnadionne@google.com>
Co-authored-by: Lidi Zheng <lidiz@google.com>
Co-authored-by: Jan Tattermusch <jtattermusch@users.noreply.github.com>
Co-authored-by: sanjaypujare <sanjaypujare@users.noreply.github.com>
Co-authored-by: Sergii Tkachenko <sergiitk@google.com>
Co-authored-by: Esun Kim <veblush@google.com>
Co-authored-by: Terry Wilson <terrymwilson@gmail.com>
Co-authored-by: Richard Belleville <rbellevi@google.com>

* added perf_counters.cc manually since the script didn't work

Co-authored-by: Mark D. Roth <roth@google.com>
Co-authored-by: donnadionne <donnadionne@google.com>
Co-authored-by: Lidi Zheng <lidiz@google.com>
Co-authored-by: Jan Tattermusch <jtattermusch@users.noreply.github.com>
Co-authored-by: sanjaypujare <sanjaypujare@users.noreply.github.com>
Co-authored-by: Sergii Tkachenko <sergiitk@google.com>
Co-authored-by: Esun Kim <veblush@google.com>
Co-authored-by: Terry Wilson <terrymwilson@gmail.com>
Co-authored-by: Richard Belleville <rbellevi@google.com>
2021-10-27 08:55:42 -07:00
Mark D. Roth 06b5061d35
change CSDS to populate new generic_xds_configs field (#27794) 2021-10-26 13:24:27 -07:00
Yash Tibrewal 9ac9a013fc
Update RDS parsing for use on servers (#27715)
* Update RDS parsing for use on servers

* Unused variable

* Reviewer comments

* Automated change: Fix sanity tests

* Fix tests

* Reviewer comments

* Reviewer comments

* clang-tidy

* Reviewer comments

* Fix test

* Reviewer comments

* Reviewer comments

Co-authored-by: yashykt <yashykt@users.noreply.github.com>
2021-10-26 00:32:35 -07:00