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

GH-44337: [CI][GLib] Fix a flaky StreamDecoder and Buffer test #44341

Merged
merged 1 commit into from
Oct 9, 2024

Conversation

kou
Copy link
Member

@kou kou commented Oct 8, 2024

Rationale for this change

It's related to GC.

StreamDecoder accepts incomplete data. They are kept until enough data are provided. A caller must not release the incomplete data before they are processed. If they are released, StreamDecoder may touch unexpected data.

What changes are included in this PR?

Refer unprocessed data until they are processed.

Are these changes tested?

Yes.

Are there any user-facing changes?

No.

It's related to GC.

StreamDecoder accepts incomplete data. They are kept until enough data
are provided. A caller must not release the incomplete data before
they are processed. If they are released, StreamDecoder may touch
unexpected data.
Copy link

github-actions bot commented Oct 8, 2024

⚠️ GitHub issue #44337 has been automatically assigned in GitHub to PR creator.

@kou
Copy link
Member Author

kou commented Oct 8, 2024

@github-actions crossbow submit verify-rc-source-ruby-*

@github-actions github-actions bot added the awaiting committer review Awaiting committer review label Oct 8, 2024
Copy link

github-actions bot commented Oct 8, 2024

Failed to push updated references, potentially because of credential issues: ['refs/heads/actions-48ff79b7c1-github-verify-rc-source-ruby-linux-ubuntu-20.04-amd64', 'refs/tags/actions-48ff79b7c1-github-verify-rc-source-ruby-linux-ubuntu-20.04-amd64', 'refs/heads/actions-48ff79b7c1-github-verify-rc-source-ruby-macos-arm64', 'refs/tags/actions-48ff79b7c1-github-verify-rc-source-ruby-macos-arm64', 'refs/heads/actions-48ff79b7c1-github-verify-rc-source-ruby-linux-conda-latest-amd64', 'refs/tags/actions-48ff79b7c1-github-verify-rc-source-ruby-linux-conda-latest-amd64', 'refs/heads/actions-48ff79b7c1-github-verify-rc-source-ruby-linux-ubuntu-22.04-amd64', 'refs/tags/actions-48ff79b7c1-github-verify-rc-source-ruby-linux-ubuntu-22.04-amd64', 'refs/heads/actions-48ff79b7c1-github-verify-rc-source-ruby-macos-amd64', 'refs/tags/actions-48ff79b7c1-github-verify-rc-source-ruby-macos-amd64', 'refs/heads/actions-48ff79b7c1-github-verify-rc-source-ruby-linux-almalinux-8-amd64', 'refs/tags/actions-48ff79b7c1-github-verify-rc-source-ruby-linux-almalinux-8-amd64', 'refs/heads/actions-48ff79b7c1']
The Archery job run can be found at: https://github.com/apache/arrow/actions/runs/11244101335

@@ -79,8 +79,15 @@ def test_consume_bytes
end

def test_consume_buffer
# We need to keep data that aren't processed yet.
Copy link
Member

Choose a reason for hiding this comment

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

Does it make any sense to proactively change test_consume_bytes too? Line 71 in this file.

Copy link
Member Author

Choose a reason for hiding this comment

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

No. We don't need to change test_consume_bytes.
arrow::ipc::StreamDecoder::Consume(const uint8_t* data, int64_t size) copied the given data internally when they aren't enough size.
(arrow::ipc::StreamDecoder::Consume(std::shared_ptr<Buffer> buffer) refers them instead of copying.)

Copy link
Member

Choose a reason for hiding this comment

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

Why doesn't the Buffer object keep the Ruby object alive? PyArrow wouldn't have this problem AFAIR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, we can do it by creating a sub Buffer class: #44349

Copy link

github-actions bot commented Oct 8, 2024

Failed to push updated references, potentially because of credential issues: ['refs/heads/actions-72b11b24d0-github-verify-rc-source-ruby-linux-ubuntu-20.04-amd64', 'refs/tags/actions-72b11b24d0-github-verify-rc-source-ruby-linux-ubuntu-20.04-amd64', 'refs/heads/actions-72b11b24d0-github-verify-rc-source-ruby-macos-arm64', 'refs/tags/actions-72b11b24d0-github-verify-rc-source-ruby-macos-arm64', 'refs/heads/actions-72b11b24d0-github-verify-rc-source-ruby-linux-ubuntu-22.04-amd64', 'refs/tags/actions-72b11b24d0-github-verify-rc-source-ruby-linux-ubuntu-22.04-amd64', 'refs/heads/actions-72b11b24d0-github-verify-rc-source-ruby-linux-almalinux-8-amd64', 'refs/tags/actions-72b11b24d0-github-verify-rc-source-ruby-linux-almalinux-8-amd64', 'refs/heads/actions-72b11b24d0-github-verify-rc-source-ruby-macos-amd64', 'refs/tags/actions-72b11b24d0-github-verify-rc-source-ruby-macos-amd64', 'refs/heads/actions-72b11b24d0-github-verify-rc-source-ruby-linux-conda-latest-amd64', 'refs/tags/actions-72b11b24d0-github-verify-rc-source-ruby-linux-conda-latest-amd64', 'refs/heads/actions-72b11b24d0']
The Archery job run can be found at: https://github.com/apache/arrow/actions/runs/11244101335

@github-actions github-actions bot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Oct 8, 2024
@kou
Copy link
Member Author

kou commented Oct 8, 2024

Hmm... I don't know why I couldn't run Crossbow jobs...

@assignUser Do you have any idea? (Is CROSSBOW_GITHUB_TOKEN expired?)

@kou
Copy link
Member Author

kou commented Oct 9, 2024

Anyway, I'll merge this because the test is also covered CI jobs in apache/arrow.

@kou kou merged commit 38c1286 into apache:main Oct 9, 2024
11 checks passed
@kou kou removed the awaiting changes Awaiting changes label Oct 9, 2024
@kou kou deleted the glib-test-decoder-buffer branch October 9, 2024 00:25
@assignUser
Copy link
Member

@kou yeah the crossbow token might have expired I'll check it out

@github-actions github-actions bot added the awaiting committer review Awaiting committer review label Oct 9, 2024
Copy link

After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 38c1286.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 6 possible false positives for unstable benchmarks that are known to sometimes produce them.

@github-actions github-actions bot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Oct 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants