Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refine StreamLoadPipe::append_and_flush() #5449

Merged
merged 1 commit into from
Apr 25, 2022

Conversation

sduzh
Copy link
Contributor

@sduzh sduzh commented Apr 24, 2022

What type of PR is this:

  • bug
  • feature
  • enhancement
  • others

Summary

Should flush the data in _write_buf before writing the data passed by append_and_flush

@sduzh sduzh requested review from chaoyli and rickif April 24, 2022 06:18
@@ -200,41 +204,8 @@ class StreamLoadPipe : public MessageBodySink, public FileReader {
}

private:
// read the next buffer from _buf_queue
Status _read_next_buffer(std::unique_ptr<uint8_t[]>* out, size_t* out_cap, size_t* out_sz, size_t padding) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice catch. I'm about to delete this.

@sduzh sduzh requested a review from wyb April 24, 2022 06:46
@rickif
Copy link
Contributor

rickif commented Apr 24, 2022

Could Status append(const ByteBufferPtr& buf) also be removed?

@sduzh
Copy link
Contributor Author

sduzh commented Apr 24, 2022

Could Status append(const ByteBufferPtr& buf) also be removed?

The unit test still relies on this method now.

@sduzh sduzh force-pushed the stream-load-pipe branch from eb36189 to ede6b34 Compare April 24, 2022 08:16
@sduzh
Copy link
Contributor Author

sduzh commented Apr 24, 2022

Could Status append(const ByteBufferPtr& buf) also be removed?

The unit test still relies on this method now.

Removed, PTAL

@sduzh sduzh requested a review from imay April 24, 2022 09:38
@sduzh sduzh merged commit f6c367d into StarRocks:main Apr 25, 2022
@sduzh sduzh deleted the stream-load-pipe branch April 25, 2022 02:58
blackstar-baba pushed a commit to blackstar-baba/starrocks that referenced this pull request Apr 28, 2022
jaogoy pushed a commit to jaogoy/starrocks that referenced this pull request Nov 15, 2023
jaogoy pushed a commit to jaogoy/starrocks that referenced this pull request Nov 15, 2023
jaogoy pushed a commit to jaogoy/starrocks that referenced this pull request Nov 15, 2023
…5451)

(cherry picked from commit 32b9ce6)

Co-authored-by: Dan Jing <jingdan@starrocks.com>
jaogoy pushed a commit to jaogoy/starrocks that referenced this pull request Nov 15, 2023
…tarRocks#5452) (StarRocks#5454)

This reverts commit 32b9ce6.

(cherry picked from commit 059dcee)

Co-authored-by: 絵空事スピリット <wanglichen@starrocks.com>
jaogoy pushed a commit to jaogoy/starrocks that referenced this pull request Nov 15, 2023
…5450)

(cherry picked from commit 32b9ce6)

Co-authored-by: Dan Jing <jingdan@starrocks.com>
jaogoy pushed a commit to jaogoy/starrocks that referenced this pull request Nov 15, 2023
…tarRocks#5452) (StarRocks#5453)

This reverts commit 32b9ce6.

(cherry picked from commit 059dcee)

Co-authored-by: 絵空事スピリット <wanglichen@starrocks.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