Closed SaiedKazemi closed 3 years ago
Changes Missing Coverage | Covered Lines | Changed/Added Lines | % | ||
---|---|---|---|---|---|
caller.go | 19 | 22 | 86.36% | ||
connectionlistener/connectionlistener.go | 43 | 46 | 93.48% | ||
<!-- | Total: | 86 | 92 | 93.48% | --> |
Files with Coverage Reduction | New Missed Lines | % | ||
---|---|---|---|---|
caller.go | 8 | 81.03% | ||
<!-- | Total: | 8 | --> |
Totals | |
---|---|
Change from base Build 349: | -1.7% |
Covered Lines: | 488 |
Relevant Lines: | 510 |
.dockerignore, line 1 at r1 (raw file):
Is this intended or accidental?
This is intended. It excludes all directories called "local", created during testing, from the context sent to the Docker daemon.
caller_test.go, line 48 at r1 (raw file):
BTW, it is unclear to me whether this test passes if main exits with rtx.Must
This has always been there and, so far, it hasn't been an issue. That said, I'd like to rework some parts of call_test.go but it's not urgent right now.
connection/connection.go, line 82 at r1 (raw file):
How about NewLocalRemoteLabeller?
NewLocalIPs was actually the name that we came up with a few weeks ago. NewLocalRemoteLabeller is too long and difficult to read. I think NewLocalRemoteIPs is a good compromise and I will change it that.
connectionlistener/connectionlistener.go, line 29 at r1 (raw file):
Add comment specifying whether ipCache, creator, and hopAnnotator are immutable after creation, and if not, how they are managed. (I might not have noticed if you hadn't changed the name of the lock.)
These are explained in ipcache.New(), connection.NewLocalRemoteIPs(), and hopannotation.New() respectively. To avoid multiple explanations that can get out of sync and confusing, I added a comment to refer to these functions for details.
connectionlistener/connectionlistener.go, line 45 at r1 (raw file):
If this only protects the map, then move it immediately above the map access.
The original code locks it before calling cl.creator.FromSockID(*id) and I didn't want to change that logic. I will add a TODO comment to revisit this and determine if we can move the lock to right before map access.
connectionlistener/connectionlistener.go, line 62 at r1 (raw file):
I really can hardly wait until the connection map goes away!
Me too :)
connectionlistener/connectionlistener_test.go, line 62 at r1 (raw file):
I think this would be clearer with a map from cookie to filename, rather than switch.
Done.
hopannotation/hopannotation.go, line 1 at r1 (raw file):
Since I'm quite tired, how about making this a trivial type with empty methods bodies, or maybe no methods at all - just type HopCache struct{} with no methods?
This file will be soon be replaced by the real implementation but I made changed it to empty structs as you've suggested.
tracer/scamper.go, line 42 at r1 (raw file):
Can this use a different error type for an existing error metric? Might be best to add the metric.Inc() now, here and above.
Done.
tracer/scamper.go, line 83 at r1 (raw file):
Yay!!
Done.
connectionlistener/connectionlistener.go, line 79 at r1 (raw file):
traceAnnotateAndArchive. Otherwise it is a bit unclear how to interpret the name.
Done.
connectionlistener/connectionlistener_test.go, line 33 at r1 (raw file):
This is a bit obscure. Can you improve the comment to be more clear?
Done.
Although this commit seems big, it does not change the behavior of traceroute-caller (hence this work was done in feature branch sandbox-saied-noop). Here's a summary of the changes:
The changes were tested with "go test ./... -race" and "docker-compose up" on my local workstation.
This change is![Reviewable](https://reviewable.io/review_button.svg)