Skip to content

buffer: add chunker#16591

Closed
daixiang0 wants to merge 12 commits intoenvoyproxy:mainfrom
daixiang0:chunker
Closed

buffer: add chunker#16591
daixiang0 wants to merge 12 commits intoenvoyproxy:mainfrom
daixiang0:chunker

Conversation

@daixiang0
Copy link
Copy Markdown
Member

@daixiang0 daixiang0 commented May 20, 2021

Signed-off-by: Long Dai long0dai@foxmail.com

Commit Message:

Add chunker class to manage the chunking process.

First part of #16139

Additional Description:
Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Deprecated:]
[Optional API Considerations:]

@daixiang0 daixiang0 force-pushed the chunker branch 4 times, most recently from 6a4196e to 3e4620d Compare May 20, 2021 08:50
jmarantz
jmarantz reviewed May 20, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@jmarantz jmarantz left a comment

Choose a reason for hiding this comment

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

looks like a good start!

@jmarantz jmarantz self-assigned this May 20, 2021
jmarantz
jmarantz reviewed May 20, 2021
View reviewed changes
@jmarantz
Copy link
Copy Markdown
Contributor

jmarantz commented May 23, 2021

/wait

jmarantz
jmarantz reviewed May 25, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@jmarantz jmarantz left a comment

Choose a reason for hiding this comment

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

Looks like this is still in progress. Can you ping when you have a working implementation of Chunker? Even if it just delegates to Buffer::Instance and doesn't actually do any chunking, this might be worth putting into the codebase as an intermediate point.

You could actually develop a real Chunker in parallel, in a separate PR, without integrating it into Admin. It should be fully testable with unit tests and mocks.

/wait

@daixiang0 daixiang0 marked this pull request as draft May 26, 2021 02:48
@daixiang0
Copy link
Copy Markdown
Member Author

daixiang0 commented May 26, 2021

/retest

@repokitteh-read-only
Copy link
Copy Markdown

repokitteh-read-only bot commented May 26, 2021

Retrying Azure Pipelines:
Check envoy-presubmit isn't fully completed, but will still attempt retrying.
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #16591 (comment) was created by @daixiang0.

see: more, trace.

@daixiang0
Copy link
Copy Markdown
Member Author

daixiang0 commented May 27, 2021

/retest

@repokitteh-read-only
Copy link
Copy Markdown

repokitteh-read-only bot commented May 27, 2021

Retrying Azure Pipelines:
Check envoy-presubmit isn't fully completed, but will still attempt retrying.
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #16591 (comment) was created by @daixiang0.

see: more, trace.

@daixiang0
Copy link
Copy Markdown
Member Author

daixiang0 commented May 27, 2021

/retest

@repokitteh-read-only
Copy link
Copy Markdown

repokitteh-read-only bot commented May 27, 2021

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #16591 (comment) was created by @daixiang0.

see: more, trace.

@daixiang0 daixiang0 marked this pull request as ready for review May 27, 2021 07:19
@daixiang0
Copy link
Copy Markdown
Member Author

daixiang0 commented May 27, 2021 •
edited
Loading

@jmarantz hi, I have done first step: use Buffer:Chunker instead without chunking, please review it, thanks a lot.

after review I will remove unrelated changes like devcontainer.json, since flake in Windoes CI..

I will try to implement chunking at local.

jmarantz
jmarantz reviewed May 27, 2021
View reviewed changes
source/common/buffer/buffer_impl.h Outdated
Copy link
Copy Markdown
Contributor

@jmarantz jmarantz May 27, 2021

Choose a reason for hiding this comment

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

We shouldn't need to change BufferImpl at all, or add the complexity of multiple inheritance here.

Copy link
Copy Markdown
Member Author

@daixiang0 daixiang0 May 31, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I met an issue in include/envoy/server/admin.h:

Http::Code AdminImpl::request(absl::string_view path_and_query, absl::string_view method,
                              Http::ResponseHeaderMap& response_headers, std::string& body) {
  AdminFilter filter(createCallbackFunction());

  auto request_headers = Http::RequestHeaderMapImpl::create();
  request_headers->setMethod(method);
  filter.decodeHeaders(*request_headers, false);
  Buffer::OwnedImpl response;   <===

  Http::Code code = runCallback(path_and_query, response_headers, response, filter);   <===
  Utility::populateFallbackResponseHeaders(code, response_headers);
  body = response.toString();
  return code;
}

Chunker can not use anything in Buffer::OwnedImpl, I do not find a good way to do it :(

Copy link
Copy Markdown
Contributor

@jmarantz jmarantz May 31, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I think Chunker could contain a Buffer::OwnedImpl (not inherit from it), and delegate to it. draining as needed to keep it under some max size. It will need to be passed, in its constructor, a network socket it can use to drain bytes as it hits its buffering limit.

If this is seeming too complex at this point we can assign someone else to do it. The work you've done so far is actually pretty helpful and you could finish that off by just (for now) making "Chunker" an alias for buffer in admin.h:

using Chunker = Buffer::Instance;

and then in a further PR we can actually make a real implementation of Chunker that auto-drains into the network.

Copy link
Copy Markdown
Member Author

@daixiang0 daixiang0 May 31, 2021

Choose a reason for hiding this comment

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

Got it, thanks for your patience and help, I learn a lot from you @jmarantz !

@jmarantz
Copy link
Copy Markdown
Contributor

jmarantz commented May 28, 2021

/wait

daixiang0 added 4 commits May 30, 2021 18:39
@daixiang0
custom dev container
b4867bb 
Signed-off-by: Long Dai <long0dai@foxmail.com>
@daixiang0
buffer: add chunker
9e2e16b 
Signed-off-by: Long Dai <long0dai@foxmail.com>
@daixiang0
1
dc8f238 
Signed-off-by: Long Dai <long0dai@foxmail.com>
@daixiang0
1
60ee7d6 
Signed-off-by: Long Dai <long0dai@foxmail.com>
@daixiang0
feedbeack
660a1af 
Signed-off-by: Long Dai <long0dai@foxmail.com>
@daixiang0
use alias
4670ba7 
Signed-off-by: Long Dai <long0dai@foxmail.com>
@daixiang0
update tests
eb2e568 
Signed-off-by: Long Dai <long0dai@foxmail.com>
@daixiang0
Copy link
Copy Markdown
Member Author

daixiang0 commented Jun 1, 2021

/retest

@repokitteh-read-only
Copy link
Copy Markdown

repokitteh-read-only bot commented Jun 1, 2021

Retrying Azure Pipelines:
Check envoy-presubmit isn't fully completed, but will still attempt retrying.
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #16591 (comment) was created by @daixiang0.

see: more, trace.

@daixiang0
Copy link
Copy Markdown
Member Author

daixiang0 commented Jun 1, 2021

/retest

@repokitteh-read-only
Copy link
Copy Markdown

repokitteh-read-only bot commented Jun 1, 2021

Retrying Azure Pipelines:
Check envoy-presubmit isn't fully completed, but will still attempt retrying.
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #16591 (comment) was created by @daixiang0.

see: more, trace.

@daixiang0 daixiang0 marked this pull request as draft June 1, 2021 09:16
@jmarantz
Copy link
Copy Markdown
Contributor

jmarantz commented Jun 2, 2021 •
edited
Loading

This looks like it's getting close. Would you ind creating a Google Doc describing in more detail the overall strategy of what we're trying to do? That will put this PR in context and perhaps make it easier for senior maintainers to review after I'm done. They may also have important comments about the strategy.

It also might help to start working on a real implementation of Chunker in parallel in a different PR. It's quite possible there's already code like this in Envoy. Maybe @alyssawilk may know where. My suggestion is to structure it something like this:

class Chunker {
 public:
  // Function to call when enough output added has exceeded size_bytes passed into the constructor, or
  // Returns false if there is a network error.
  using FlushFn = std::function<bool(absl::string_view)>;

  Chunker(FlushFn flusher, uint32_t size_bytes);

  // Adds bytes to an owned buffer until size_bytes is reached, then flushes the remaining bytes to the network,
  // returning false if a flush occurred and failed.
  bool add(absl::string_view);

  // Forces a flush. Usually called before destructing.
  bool flush();

  // Reports an error as text sent to the flusher. Calls to add or flush after reportError are ignored.
  bool reportError(int error_code, absl::string_view message);
};

This class can be written and tested in a tight unit test without doing any networking or worrying about admin semantics. The unit test can supply a flusher that doesn't actually access the network.

Assuming this class (or a similar one) isn't already implemented somewhere.

@daixiang0
Copy link
Copy Markdown
Member Author

daixiang0 commented Jun 2, 2021

I would do that, maybe need some time to finish.

@daixiang0
Copy link
Copy Markdown
Member Author

daixiang0 commented Jun 2, 2021

/retest

@repokitteh-read-only
Copy link
Copy Markdown

repokitteh-read-only bot commented Jun 2, 2021

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #16591 (comment) was created by @daixiang0.

see: more, trace.

@daixiang0 daixiang0 marked this pull request as ready for review June 2, 2021 07:39
@daixiang0
Copy link
Copy Markdown
Member Author

daixiang0 commented Jun 2, 2021

@jmarantz CI pass, please review again, thanks

jmarantz
jmarantz reviewed Jun 2, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@jmarantz jmarantz left a comment

Choose a reason for hiding this comment

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

This looks fine now and can potentially be merged as is. However I think to do that we need buy-in from a @envoyproxy/senior-maintainers on the plan first. Once we have that available & approved we can merge this.

In the meantime you can start working in a parallel PR on a Chunker implementation.

@jmarantz
Copy link
Copy Markdown
Contributor

jmarantz commented Jun 2, 2021

/wait

@daixiang0 daixiang0 mentioned this pull request Jun 15, 2021
Closed
@github-actions
Copy link
Copy Markdown

github-actions bot commented Jul 2, 2021

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@github-actions github-actions bot added the stale stalebot believes this issue/PR has not been touched recently label Jul 2, 2021
@daixiang0
Copy link
Copy Markdown
Member Author

daixiang0 commented Jul 5, 2021

/wait

@github-actions github-actions bot removed the stale stalebot believes this issue/PR has not been touched recently label Jul 5, 2021
@github-actions
Copy link
Copy Markdown

github-actions bot commented Aug 4, 2021

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@github-actions github-actions bot added the stale stalebot believes this issue/PR has not been touched recently label Aug 4, 2021
@github-actions
Copy link
Copy Markdown

github-actions bot commented Aug 11, 2021

This pull request has been automatically closed because it has not had activity in the last 37 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@github-actions github-actions bot closed this Aug 11, 2021
@daixiang0 daixiang0 deleted the chunker branch August 12, 2021 00:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jmarantz jmarantz jmarantz left review comments

Assignees

@jmarantz jmarantz

Labels

stale stalebot believes this issue/PR has not been touched recently waiting

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@daixiang0 @jmarantz