search

gyscos / zstd-rs

A rust binding for the zstd compression library.
MIT License
510 stars 107 forks source link

Allow environment variable to specify number of threads for compression #256

Closed orhun closed 8 months ago

orhun commented 8 months ago

See https://github.com/facebook/zstd/pull/2299

I think it would be nice to have it here as well. I'm interested in contributing if you find this applicable 🐻

gyscos commented 8 months ago

Hi, and thanks for the report!

I believe the linked PR is for the CLI binary, not for the lib itself, right?

I think the bindings should more closely match the lib that the CLI.

The lib (and these bindings) can potentially be used internally by other tools with their own parallelism configuration; affecting all these with an env variable initially intended for a (mostly) unrelated CLI sounds a bit intrusive.

In particular this env variable sounds like a way to specifically control the zstd CLI. Even if someone was writing a new CLI in rust using zstd-rs, it's not guaranteed they'd want to support the same env variables. This is why I'm not even sure it makes sense to add an optional, opt-in function that load parameters from these env variables. I just don't see many use cases.

If you do have examples where it would make sense to support this, I'll be glad to be proven wrong!

orhun commented 8 months ago

You're right! I overlooked that it is a feature in the CLI part of zstd and not the library. It makes total sense to not add such configuration options to the library and leave that to the user.

Closing...

P.S. Thanks for the great project!