Increase the check interval

[yeasy]

Type of change

• Test update

Description

This patch increase the check interval from the default 1ms to 1s when
updating channel config.

Originally, there will emit lots of useless checking when the channel is
not ready temporarily, and waste cpu utilization.

Using 1 s as the checking period is good enough for the integration test.

[sykesm]

I understand why you want to make this change but given how narrowly scoped it is, it's not something that I'd support merging.

The eventually loops all use the default polling time from gomega. If you're willing to wait longer, ginkgo has functions that control the default times.

[yeasy]

I understand why you want to make this change but given how narrowly scoped it is, it's not something that I'd support merging.

The eventually loops all use the default polling time from gomega. If you're willing to wait longer, ginkgo has functions that control the default times.

The method improved here is a little different from others, which emits a deliver request to the OSN every 1 ms. And it's noticed that the CPU is quite high during this polling.

note (@sykesm): I mistakenly edited this comment earlier instead of replying. Apologies. I believe I've restored the original comment from history. It was not my intent to change your words.

[sykesm]

The method improved here is a little different from others

I really don't see how. This Eventually drives CurrentConfigBlock with a delay of 10ms between calls. This is the same polling interval that is used everywhere. It's repeatedly calling peer channel fetch to get the height. The same pattern is used when calling peer lifecycle chaincode is used for checkcommitreadiness or queryinstalled, and when discover is used to find endorsers. They all turn into tight loops of CLI tools calling the servers to get data.

which emits a deliver request to the OSN every 1 ms. And it's noticed that the CPU is quite high during this polling.

10 ms, and yes, CPU is high during all poling in the integration tests. This is why I understand why you're suggesting the change and why I'm saying that the scope is so narrow as to not be helpful.

Simply changing random Eventually calls isn't going to help with maintenance, clarity, or overall CPU consumption; changes need to be less ad-hoc and more systematic.

[yeasy]

The method improved here is a little different from others

I really don't see how. This Eventually drives CurrentConfigBlock with a delay of 10ms between calls. This is the same polling interval that is used everywhere. It's repeatedly calling peer channel fetch to get the height. The same pattern is used when calling peer lifecycle chaincode is used for checkcommitreadiness or queryinstalled, and when discover is used to find endorsers. They all turn into tight loops of CLI tools calling the servers to get data.

which emits a deliver request to the OSN every 1 ms. And it's noticed that the CPU is quite high during this polling.

10 ms, and yes, CPU is high during all poling in the integration tests. This is why I understand why you're suggesting the change and why I'm saying that the scope is so narrow as to not be helpful.

Simply changing random Eventually calls isn't going to help with maintenance, clarity, or overall CPU consumption; changes need to be less ad-hoc and more systematic.

Well, not sure whether it's a good idea to change the code at the same time without this issue happens.

But sure, we can modify the global default polling interval of the test suite.

[sykesm]

Thanks for raising the concern via PR. I've proposed #1111 to address this at a scope that should address polling CLI operations without impacting other parts of the integration tests.