Skip to content

Add quic_client_stats_impl.h to QUICHE platform implementation#5832

Merged
alyssawilk merged 5 commits intoenvoyproxy:masterfrom
wu-bin:quic_platform_client_stats
Feb 5, 2019
Merged

Add quic_client_stats_impl.h to QUICHE platform implementation#5832
alyssawilk merged 5 commits intoenvoyproxy:masterfrom
wu-bin:quic_platform_client_stats

Conversation

@wu-bin
Copy link
Contributor

@wu-bin wu-bin commented Feb 4, 2019

Description:

Add quic_client_stats_impl.h to QUICHE platform implementation, with everything being no-op.

Risk Level: minimal: code not used yet
Testing: bazel test test/extensions/quic_listeners/quiche/platform:quic_platform_test --test_output=all
Docs Changes: none
Release Notes: none
[Optional Fixes #Issue]
[Optional Deprecated:]

everything being no-op.

Signed-off-by: Bin Wu <wub@google.com>
@wu-bin
Copy link
Contributor Author

wu-bin commented Feb 4, 2019

/assign @mpwarres

mpwarres
mpwarres previously approved these changes Feb 4, 2019
Copy link
Contributor

@mpwarres mpwarres left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Signed-off-by: Bin Wu <wub@google.com>
Signed-off-by: Bin Wu <wub@google.com>
@wu-bin
Copy link
Contributor Author

wu-bin commented Feb 4, 2019

/retest

@repokitteh-read-only
Copy link

🔨 rebuilding ci/circleci: mac (failed build)

🐱

Caused by: a #5832 (comment) was created by @wu-bin.

see: more, trace.

enum class TestEnum { ZERO = 0, ONE, TWO, COUNT };

TEST(QuicPlatformTest, QuicClientStats) {
// Just make sure they compile.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On functions where we're not implementing the actual functionality, should we have comments or a document somewhere for what expect to work and what not?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can 1) add comments to quic_client_stats_impl.h, 2) add a column to the platform impl tracking spreadsheet to say it's not actually implemented.

Or both? WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd mildly prefer something in the code just so folks don't have to dig up the spreadsheet but as long as we're tracking it I'm good :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a comment in quic_client_stats_impl.h.

@htuch
Copy link
Member

htuch commented Feb 4, 2019

Please merge master to pick up #5827.

…ent_stats

Signed-off-by: Bin Wu <wub@google.com>
@wu-bin
Copy link
Contributor Author

wu-bin commented Feb 4, 2019

Please merge master to pick up #5827.

Done.

@alyssawilk alyssawilk merged commit 5361b77 into envoyproxy:master Feb 5, 2019
fredlas pushed a commit to fredlas/envoy that referenced this pull request Mar 5, 2019
…proxy#5832)

Add quic_client_stats_impl.h to QUICHE platform implementation, with everything being no-op.

_Risk Level_: minimal: code not used yet
_Testing_: bazel test test/extensions/quic_listeners/quiche/platform:quic_platform_test --test_output=all
_Docs Changes_: none
_Release Notes_: none

Signed-off-by: Bin Wu <wub@google.com>
Signed-off-by: Fred Douglas <fredlas@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants