Skip to content

fix(protobuf-core): re-introduce the load-by-file-path/url protobuf descriptors #18770

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.

Sign up for GitHub

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

Jump to bottom
Merged
gianm merged 2 commits into from
Nov 25, 2025
Merged

fix(protobuf-core): re-introduce the load-by-file-path/url protobuf descriptors #18770

gianm merged 2 commits into from
Nov 25, 2025

Conversation

@hfukada
Copy link
Contributor

@hfukada hfukada commented Nov 21, 2025 •
edited
Loading

Description

Druid 35.0.0 broke URL path loading for protobufs. Reintroduce this feature.

Release note

Reintroduce URI protobuf descriptor files loading.

This PR has:

  • been self-reviewed.
  • added documentation for new or modified features or behaviors.
  • a release note entry in the PR description.
  • added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
  • added or updated version, license, or notice information in licenses.yaml
  • added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
  • added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
  • added integration tests.
  • been tested in a test Druid cluster.

amaechler
amaechler reviewed Nov 21, 2025
View reviewed changes
Copy link
Contributor

@amaechler amaechler left a comment

Choose a reason for hiding this comment

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

Thanks for the fix! One small comment around resource handling, otherwise lgtm.

...ns/src/test/java/org/apache/druid/data/input/protobuf/FileBasedProtobufBytesDecoderTest.java Outdated
}

@Test
public void tesDescriptorUrl()
Copy link
Contributor

@amaechler amaechler Nov 21, 2025

Choose a reason for hiding this comment

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

Suggested change
public void tesDescriptorUrl()
public void testDescriptorUrl()

...nsions/src/main/java/org/apache/druid/data/input/protobuf/FileBasedProtobufBytesDecoder.java Outdated
Comment on lines 76 to 78
try {
fin = url.openConnection().getInputStream();
}
Copy link
Contributor

@amaechler amaechler Nov 21, 2025

Choose a reason for hiding this comment

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

Do we need some resource handling for this inputstream in case DescriptorProtos.FileDescriptorSet.parseFrom fails below and properly close the stream again? Maybe just by refactoring a bit and then doing try (fin = url.openConnection().getInputStream()) { ...

@hfukada hfukada requested a review from amaechler November 21, 2025 19:13
@hfukada
Copy link
Contributor Author

hfukada commented Nov 24, 2025

@amaechler can you re-look? the unit test failures are not my own changes.

amaechler
amaechler approved these changes Nov 25, 2025
View reviewed changes
Copy link
Contributor

@amaechler amaechler left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks for the fix. 🐻

gianm
gianm approved these changes Nov 25, 2025
View reviewed changes
Copy link
Contributor

@gianm gianm left a comment

Choose a reason for hiding this comment

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

I just restarted the test that was failing. Maybe it was flaky.

@gianm gianm merged commit 60ad692 into apache:master Nov 25, 2025
71 of 72 checks passed
@hfukada
Copy link
Contributor Author

hfukada commented Nov 28, 2025

can we backport this change to 35.0.0?

@clintropolis clintropolis added this to the 35.0.1 milestone Dec 1, 2025
clintropolis pushed a commit that referenced this pull request Dec 1, 2025
@hfukada @clintropolis
fix(protobuf-core): re-introduce the load-by-file-path/url protobuf d…
0da44fe 
…escriptors (#18770)

* fix(protobuf-core): re-introduce the load-by-file-path/url protobuf descriptors

* fix: typo and refactor with context
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gianm gianm gianm approved these changes

+1 more reviewer

@amaechler amaechler amaechler approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

35.0.1

Development

Successfully merging this pull request may close these issues.

4 participants

@hfukada @gianm @amaechler @clintropolis