Srikanth Chekuri
Srikanth Chekuri
We don't make it clear anywhere that `ReceiverLoop` is an internal implementation detail. By making both connection closing and context cancellation (I don't see much use of received parent context...
I recall we had discussed this already once. The implementation can always evolve to satisfy the needs but We should explicitly call out what we consider as internal implementation details.
I am not sure I follow the simplicity part. I don't see much benefit of one over the other. Regarding the memory leak concern - as we are going to...
Please add a test case. Let's try to include cases one where the installation of the new version fails and another that succeeds.
Regardless of the size of the change, we should add tests. That's how we prove the change actually fixes it and having a test ensures the issue will not appear...
@phanidevavarapu I had opened a PR to add tests against your fork here https://github.com/phanidevavarapu/opamp-go/pull/1. Did you get a chance to look at it?
Can you fix the conflicts and update the packagessyncer.go because I commented out the lines to show tests fail.
While the individual test case `go test -timeout 30s -run ^TestOfferUpdatedVersion$ github.com/open-telemetry/opamp-go/client` passes successfully, running the entire test suite fails with a timeout failure consistently. We need to check why...
The changes look good to me. I tried running the workflow multiple times to see if shows flaky behaviour but it passed all runs. I still suspect the test is...
>I think it would be useful to try to implement restarting in our Supervisor example to get a better feel of how this machinery should work. I agree. That is...