[FAB-17184] Skip pulling from remote peers if all remaining missing rwsets are invalid when peer is configured to skip pulling invalid transactions

[caod123]

Type of change

Improvement

Description

Prior to this change pvtdataprovider would enter the pull retry loop
and still attempt to fetch from peers even if all missing rwsets were
invalid

Related issues

FAB-17184

[lindluni]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[lindluni]

Can you rebase this on master to pick up the retryThreshold increase and see if that helps

[caod123]

Can you rebase this on master to pick up the retryThreshold increase and see if that helps

Sure

[guoger]

I wonder if we could return a needToRetry bool from populateFromRemotePeers to indicate whether it fails to pull any valid transaction? so we don't necessarily need to keep track of a counter at several places

[caod123]

Force-push diff is a little messy because I also rebased off master.

[guoger]

I thought about that originally but decided against it since it seemed unintuitive that populateFromRemotePeers should return anything. I don't have any strong opinions though.

IMHO, it's naturally to expect some hints to be returned when it comes to "do something with remote peers". More importantly, i think the code would be more manageable if we don't carry struct member numInvalid all along, and alter it in both populateFromCache and populateFromTransient, especially when we have all the info needed to compute such result in populateFromRemotePeers.

here's what i have in mind guoger@aa2cbf9

let me know what you think

[caod123]

@guoger since the previous requested change is no longer valid, can you dismiss the requested change to unblock the PR?

Already rebased, no longer relevant.

[guoger]

LGTM, but would like experts in this area to take a look as well.

[C0rWin]

Looks good