Upgrade gRPC JVM libs #2276
Upgrade gRPC JVM libs #2276
Conversation
majcherm-da
requested review from
cocreature,
hurryabit,
rautenrieth-da and
nickchapman-da
| (_,hOutOpt,_,proh) <- | ||
| createProcess processRecord { | ||
| std_out = CreatePipe, | ||
| std_err = CreatePipe, -- Question: ought the pipe to be drained? |
There was a problem hiding this comment.
Now we know the answer :)
| std_out = CreatePipe, | ||
| std_err = CreatePipe, -- Question: ought the pipe to be drained? | ||
| create_group = True -- To avoid sending INT to ourself | ||
| create_group = True -- To avoid sending INT to ourself |
There was a problem hiding this comment.
I’m not sure I understand this. What exactly is the issue that you are trying to prevent here?
| (_,hOutOpt,_,proh) <- | ||
| createProcess processRecord { | ||
| std_out = CreatePipe, |
There was a problem hiding this comment.
Does this mean that we are now spamming stdout and stderr during tests? From a quick experiment it looks like sandbox is still very noisy even if you pass --port-file. Maybe just redirect stdout to /dev/null? (stderr seems to be clean by default afaict). typed-process has a cross-platform version of /dev/null that we could either steal or we could just switch to typed-process which generally seems like a saner API (we’ve already switched in other places due to issues with process).
There was a problem hiding this comment.
yes, these are captured by Bazel and put into test.log, which I thought was better to keep, but on the other hand these are mixed with test results - I can redirect them to /dev/null
There was a problem hiding this comment.
I often run tests via bazel run to be able to pass arguments and see the output as it goes so I would prefer if they are not too noisy.
| startSandbox :: SandboxSpec -> IO Sandbox | ||
| startSandbox spec = withTempFile $ \portFile -> do | ||
| (proh,_hOpt) <- startSandboxProcess spec portFile | ||
| portNum <- readPortFile maxRetries portFile |
majcherm-da
requested review from
gerolf-da and
stefanobaghino-da
as code owners
majcherm-da
force-pushed
the
upgrade_grpc_jvm
branch
from
15d62b4 to
9b1044f
Compare
The problem with upgrading JVM's gRPC lib further than 1.19.0 was caused by setting up a stdout/stderr pipe in HS bindings tests, which were not drained causing a backpressure effect on tests on Windows. Sandbox thread was blocked on: https://github.com/grpc/grpc-java/blob/v1.21.0/netty/src/main/java/io/grpc/netty/NettyServerBuilder.java#L512 which is a logger call, logging to stdout. The log line has been added in 1.21.0 of gRPC :)
I modified
Sandbox.hsto create sandbox using--port-fileoption and used standard stdout/stderr proxying to tests' stdout/stderr.Pull Request Checklist
NOTE: CI is not automatically run on non-members pull-requests for security
reasons. The reviewer will have to comment with
/AzurePipelines runtotrigger the build.