Skip to content
This repository has been archived by the owner on Oct 23, 2024. It is now read-only.

Fix CircleCI build (part 2) #193

Merged
merged 7 commits into from
Aug 1, 2015
Merged

Fix CircleCI build (part 2) #193

merged 7 commits into from
Aug 1, 2015

Conversation

tsenart
Copy link
Contributor

@tsenart tsenart commented Jul 27, 2015

This change set brings us a little closer to have a green CircleCI build. After this is merged, the remaining failures are very few:

records/generator_test.go:234::warning: cyclomatic complexity 19 of function TestInsertState() is high (> 12) (gocyclo)
main.go:19::warning: cyclomatic complexity 14 of function main() is high (> 12) (gocyclo)
resolver/resolver_test.go:193::warning: cyclomatic complexity 21 of function TestHandler() is high (> 12) (gocyclo)
resolver/resolver_test.go:336::warning: cyclomatic complexity 17 of function TestHTTP() is high (> 12) (gocyclo)
records/generator.go:167:8:warning: error return value not checked (defer resp.Body.Close()) (errcheck)

EDIT: I reduced the scope of this PR to NOT include the label package refactoring. That will follow up in a different PR. This way we can get this code in right away I hope.

@tsenart tsenart added the WIP label Jul 27, 2015
@tsenart tsenart self-assigned this Jul 27, 2015
@tsenart tsenart force-pushed the fix-build branch 4 times, most recently from 7f4aaf7 to 3378378 Compare July 28, 2015 16:23
@tsenart tsenart changed the title [WIP] Fix build (part 2) Fix CircleCI build (part 2) Jul 28, 2015
@tsenart tsenart added PTAL and removed WIP labels Jul 28, 2015
@tsenart
Copy link
Contributor Author

tsenart commented Jul 28, 2015

PTAL @sttts @jdef

@jdef
Copy link
Contributor

jdef commented Jul 28, 2015

i'm curious about the benchmarking/performance of the labels refactoring. The prior table-based lookup approach really lowered label generation times (performance was key concern of @kozyraki). The new implementation looks more involved so I'm wondering what that does to label generation times.

jdef
jdef reviewed Jul 28, 2015
View reviewed changes
circle.yml
@@ -9,5 +9,5 @@ dependencies:
test:
override:
- ! gofmt -s -d . 2>&1 | read
- gometalinter ./...
- gometalinter --cyclo-over=12 ./...
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how did we pick 12? why not 13, or 15?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The lower, the better. But at some point we cross a threshold that doesn't make sense anymore. I found this value through quite extensive experimentation.

@tsenart
Copy link
Contributor Author

tsenart commented Jul 28, 2015

I can run the benchmarks tomorrow for the two versions but I'd like to see valid data backed evidence of what real performance requirements are for label generation. I'd be very surprised if it would be a bottleneck and I'd optimise for maintainability, composability and modularity first.

@tsenart tsenart force-pushed the fix-build branch 3 times, most recently from 4aa950c to c72ae07 Compare July 31, 2015 07:39
@tsenart tsenart assigned jdef and unassigned tsenart Jul 31, 2015
sttts
sttts reviewed Jul 31, 2015
View reviewed changes
logging/logging.go
)

// Counter defines an interface for a monotonically incrementing counter.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sounds like a tautology. s/counter/integer/

In fact, it's a write-only counter. How can one read the value?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

users of the Counter API aren't meant to read the value. Counter values are rendered periodically in the logs, and so LogCounter.String() is invoked to display the value.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll s/counter/value/ to remove the tautology.

sttts
sttts reviewed Jul 31, 2015
View reviewed changes
resolver/resolver_test.go
if m.Rcode != 3 {
t.Error("not setting NXDOMAIN for AAAA requests")
if got, want := m.Rcode, dns.RcodeNameError; got != want {
t.Errorf("not setting NXDOMAIN for AAAA requests: got rcode: %v, want: %v", got, want)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rcode or Rcode as above? same for got/want vs. Answer.

@jdef jdef assigned sttts and unassigned jdef Jul 31, 2015
sttts
sttts reviewed Jul 31, 2015
View reviewed changes
resolver/resolver.go
}
m.Rcode = dns.RcodeNameError
if qType == dns.TypeAAAA && len(rs.SRVs[name])+len(rs.As[name]) > 0 {
m.Rcode = dns.RcodeSuccess
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code path is guarded now by len(m.Answer)==0. It wasn't before. Problem?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's OK since dns.RcodeSuccess is the default value.

@tsenart tsenart force-pushed the fix-build branch 3 times, most recently from 4554ea1 to 34b158b Compare July 31, 2015 21:12
@tsenart
Copy link
Contributor Author

tsenart commented Jul 31, 2015

@sttts: PTAL again :-)

@tsenart tsenart mentioned this pull request Aug 1, 2015
Merged
sttts
sttts reviewed Aug 1, 2015
View reviewed changes
resolver/resolver.go

}
}
if err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't the error case take precedence over the len==0 case? Now a handler which throws an error will possibly also return an empty answer and then the error never shows up.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which is what was previously happening, unfortunately. But I'll change it.

@sttts
Copy link
Contributor

sttts commented Aug 1, 2015

A few comments, otherwise lgtm

tsenart added a commit that referenced this pull request Aug 1, 2015
@tsenart
Merge pull request #193 from mesosphere/fix-build
11ecb7c 
Fix CircleCI build (part 2)
@tsenart tsenart merged commit 11ecb7c into master Aug 1, 2015
@sttts
Copy link
Contributor

sttts commented Aug 1, 2015

👍

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees

@sttts sttts

Labels
PTAL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tsenart @jdef @sttts