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

feat: Configure optional kerberos feature for Python package #91

Merged
merged 1 commit into from
Apr 2, 2024

Conversation

yjshen
Copy link
Contributor

@yjshen yjshen commented Mar 31, 2024

To make Kerberos an optional feature for python package, and make it consistent with both hdfs-native and hdfs-native-object-store:

  • Remove the explicit "kerberos" feature from the hdfs-native dependency declaration in Cargo.toml.
  • Add a new feature named "kerberos", allowing the "kerberos" feature to be optionally enabled for the Python package.

@Kimahriman
Copy link
Owner

What's the use case, building your own Python wheel? Are you trying to use on a system that currently doesn't have wheels built (i.e. not Linux or Mac x86 or aarch64)? Or just looking for a smaller custom Python build?

@yjshen
Copy link
Contributor Author

yjshen commented Mar 31, 2024

Hi @Kimahriman, thanks for your response.

The primary reason for this PR was a preparatory step before running benchmarks locally on my system. I attempted to build the entire project from scratch and noticed an inconsistency in the usage of features between the Python package and the other two components. This inconsistency wasn't about targeting systems without pre-built wheels or seeking a custom Python build for non-standard platforms. Instead, it was more about ensuring uniformity and smooth operation within my development environment.

@Kimahriman
Copy link
Owner

I gotcha. The main difference with the Python package is that it isn't published as a Cargo crate, it's just used to publish the Python package which needs to have a specific set of features at publish time. So if we want to have a feature for it still on the Python side it needs to either be set as a default feature or needs to be explicitly included in the CI at publish time. I'd lean toward the former if anything. I may consider making kerberos included by default for the other packages, especially now that I've removed the gsasl requirement and there's only one native dependency.

Let me know anything interesting from benchmarks you run! I've only recently tried to run some and had some surprisingly slow results that I fixed in the last release. I wouldn't expect including or excluding the kerberos feature to have much if any affect on run time against a cluster without security enabled, just slightly longer build times and inability to cross-compile, which is why it's not currently included by default.

@yjshen
Copy link
Contributor Author

yjshen commented Apr 1, 2024

The main difference with the Python package is that it isn't published as a Cargo crate, it's just used to publish the Python package which needs to have a specific set of features at publish time.

If this is the case, does excluding the Python package from the Cargo workspace make sense to avoid any potential confusion and ensure clear separation between Rust and Python?

Let me know anything interesting from benchmarks you run!

Thanks for the amazing work! I'll let you know my benchmark results once I have them.

@Kimahriman
Copy link
Owner

Ok that makes a lot of sense, and explains all the weird things I've seen running tests with vs without using -p to specify the project. I think I added it to the workspace to just get VSCode support working, but I think it should stay in there so I can update the common dependency versions to use the workspace version.

With all that said, I agree it should be defaulted off, and I'll just update the CI to deploy with the kerberos feature.

@Kimahriman Kimahriman merged commit adedc18 into Kimahriman:master Apr 2, 2024
7 checks passed
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.

2 participants