xdsclient: fix unexpectedly large LoadReportInterval in initial load report request#8348
Conversation
purnesh42H
requested review from
dfawley and
easwars
and removed request for
dfawley
purnesh42H
changed the title
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #8348 +/- ##
==========================================
+ Coverage 82.16% 82.29% +0.13%
==========================================
Files 419 419
Lines 42034 42052 +18
==========================================
+ Hits 34537 34607 +70
+ Misses 6023 5987 -36
+ Partials 1474 1458 -16
🚀 New features to boost your workflow:
|
purnesh42H
added
the
Area: xDS
arjan-bal
left a comment
arjan-bal
left a comment
There was a problem hiding this comment.
Can you please add a test for this to prevent regressions?
we can only check if LoadReportInterval is positive but not excessively large. We can't compare exactly since calculation involves time.Now(). I have added test verifications in the existing e2e tests with some tolerance to ensure reportInterval of the first report and subsequent reports is a small, positive duration but greater than configured server LoadReportingInterval. |
Without looking at the details here... Can we hook |
purnesh42H
force-pushed
the
load-store-init-last-report-at
branch
from
012a5bc to
6a3ebaf
Compare
Added the hook. Thought that limits us to only verify with predictable report interval in the unit test of the load store and not in the e2e tests i.e. verifying the report interval that got reported to LRS server but that's probably fine. |
purnesh42H
force-pushed
the
load-store-init-last-report-at
branch
from
6a3ebaf to
65fd12d
Compare
purnesh42H
force-pushed
the
load-store-init-last-report-at
branch
from
65fd12d to
b7c391d
Compare
If needed, you can hop through an internal package to be able to set from e2e tests. |
dfawley
left a comment
dfawley
left a comment
There was a problem hiding this comment.
LGTM, just a couple very minor things.
| wantInterval := 5 * time.Second | ||
| if stats1.reportInterval != wantInterval { |
There was a problem hiding this comment.
Optional:
I think the best way I've been recommended of writing this kind of test comparison is:
if got, want := stats1.reportInterval, 5 * time.Second; got != want {
t.Fatalf("blah blah = %v; want %v", got, want)
}| ) | ||
|
|
||
| // clockNow is used to get the current time. It can be overridden in tests. | ||
| var clockNow = time.Now |
There was a problem hiding this comment.
Sorry for the nit, but would you mind renaming this to timeNow to match packagename.SymbolName but without the dot? That's the convention we generally use.
xds/internal/xdsclient/load/store.go
Outdated
| const negativeOneUInt64 = ^uint64(0) | ||
|
|
||
| // clockNow is used to get the current time. It can be overridden in tests. | ||
| var clockNow = time.Now |
| currentTime = currentTime.Add(5 * time.Second) | ||
| stats1 := store.Stats(nil) | ||
|
|
||
| if stats1 == nil { |
There was a problem hiding this comment.
It seems like you want even more from this check, since it's a slice that you dereference element 0 of immediately below.
if len(stats1) == 0 { // or != 1 ?
t.Fatalf("store.Stats(nil) = %v; want len() >= 1", stats1) // or == 1?
}
I did it for the generic lrs client tests but left it for the internal one since that will be external soon. |
purnesh42H
force-pushed
the
load-store-init-last-report-at
branch
from
ef2a9f6 to
6bdf940
Compare
4 participants
Internal bug: b/416260484
The
lastReportedAtfield is uninitialized during reporter construction, defaulting to theZerotimestamp. This results in an excessively largeLoadReportInterval(calculated as Now() - lastReportedAt) for the initial load report. InitializinglastReportedAtto Now() during construction will ensure theLoadReportIntervalis accurately approximated for the first report.RELEASE NOTES: