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

Support for configuring NuProcess buffer capacity via user property #152

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

avrecko
Copy link
Contributor

@avrecko avrecko commented Feb 13, 2024

Configurable via property com.zaxxer.nuprocess.bufferCapacity.

I've set the limits between 1 KiB and 1 MiB. They are arbitrary choices. Should handle use cases of 4, 8, 16, 32, 64 KB/KiB.

edit: could also set limit to something like 128 KiB? Thoughts?

src/main/java/com/zaxxer/nuprocess/NuProcessBuilder.java Outdated Show resolved Hide resolved
src/main/java/com/zaxxer/nuprocess/NuProcessBuilder.java Outdated Show resolved Hide resolved
@@ -310,4 +310,26 @@ private String[] prepareEnvironment()

return env;
}

static int getGlobalBufferSizeSetting()
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think this should be here. I'd put it on NuProcess instead. If a point comes where the NuProcessBuilder grows the ability to control the buffer size per-process, NuProcess.BUFFER_CAPACITY would still not be able to reflect that (since it's a static constant). I suspect if that ever became possible we'd need to deprecate that constant and either remove it or rename it (e.g. DEFAULT_BUFFER_CAPACITY). Regardless, though, it still wouldn't need to look at the builder to know what that value is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, maybe we put this in Constants class?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also first thought about putting it in NuProcess but it is an interface. Not a big fan of implementation in interfaces. Constants feels good to me. But if you like. No problem for me to put something in NuProcess.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For more fine grained control I'd put it in the NuProcessBuilder.

NuProcessBuilder(....).setStdoutBufferCapacity(4092).setStdinBufferCapacity(32000).setStdErrBufferCapacity(16000);

src/main/java/com/zaxxer/nuprocess/NuProcessBuilder.java Outdated Show resolved Hide resolved
@bturner
Copy link
Collaborator

bturner commented Feb 13, 2024

Re: using 128K as the limit, I don't thing we need to super prescriptive here in the library. To some extent, I'm a bit inclined to just let the caller use whatever value they want, even if it's what I would consider excessively small or large, and not validate at all beyond ensuring it's a number (and defaulting the value if it isn't, rather than throwing).

If we're going to have a limit, I'd probably leave it as you have it.

@avrecko
Copy link
Contributor Author

avrecko commented Feb 13, 2024

Re: using 128K as the limit, I don't thing we need to super prescriptive here in the library. To some extent, I'm a bit inclined to just let the caller use whatever value they want, even if it's what I would consider excessively small or large, and not validate at all beyond ensuring it's a number (and defaulting the value if it isn't, rather than throwing).

If we're going to have a limit, I'd probably leave it as you have it.

I think most if not all would want to use 4, 8, 16 or 32 as alternatives to 64. So I think a limit makes sense.

@avrecko avrecko force-pushed the configurable_buffer branch 4 times, most recently from 4e1b1e3 to a2627a0 Compare February 13, 2024 20:35
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