Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Mention overflows when mistakenly call function FromInt #36487

Merged
merged 1 commit into from
Nov 29, 2016
Merged

Mention overflows when mistakenly call function FromInt #36487

merged 1 commit into from
Nov 29, 2016

Conversation

xialonglee
Copy link
Contributor

@xialonglee xialonglee commented Nov 9, 2016 •
edited by k8s-oncall
Loading

What this PR does / why we need it:
When mistakenly call this method with a value that overflows int32 will causes strange behavior in some environment (maybe in amd64 system, i'm not sure but my test shows that).
For example, call FromInt(93333333333) would result in -1155947179 and not mention overflows.

This change is Reviewable

@k8s-ci-robot
Copy link
Contributor

Can a kubernetes member verify that this patch is reasonable to test? If so, please reply with "@k8s-bot ok to test" on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands will still work. Regular contributors should join the org to skip this step.

If you have questions or suggestions related to this bot's behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-github-robot k8s-github-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. release-note-label-needed labels Nov 9, 2016
@dims
Copy link
Member

dims commented Nov 9, 2016

@k8s-bot ok to test

@xialonglee
Copy link
Contributor Author

@k8s-bot unit test this

@xialonglee
Copy link
Contributor Author

hi @dims ,i rebased just now for passing the unit test and it seems that it wouldn't trigger the checks schedule again. Would you please to start the checks again?

@dims
Copy link
Member

dims commented Nov 10, 2016

@xialonglee it says "Build started." So let's wait a bit.

@brendandburns
Copy link
Contributor

I don't see the harm in this, but I'd ask for two things:

  1. use glog not fmt
  2. print the stack trace (see debug.PrintStack) so that we know where there is a bad value.

Thanks!

@xialonglee xialonglee force-pushed the mention-overflows branch 2 times, most recently from bad8ddb to 95cbd35 Compare November 10, 2016 08:18
@k8s-ci-robot
Copy link
Contributor

Jenkins GCE Node e2e failed for commit 95cbd352e1f88fcb7bc5a27752079b412ea33d66. Full PR test history.

The magic incantation to run this job again is @k8s-bot node e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins GCI GKE smoke e2e failed for commit 95cbd352e1f88fcb7bc5a27752079b412ea33d66. Full PR test history.

The magic incantation to run this job again is @k8s-bot gci gke e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins Kubemark GCE e2e failed for commit 95cbd352e1f88fcb7bc5a27752079b412ea33d66. Full PR test history.

The magic incantation to run this job again is @k8s-bot kubemark e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins GKE smoke e2e failed for commit 95cbd352e1f88fcb7bc5a27752079b412ea33d66. Full PR test history.

The magic incantation to run this job again is @k8s-bot cvm gke e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins GCE e2e failed for commit 95cbd352e1f88fcb7bc5a27752079b412ea33d66. Full PR test history.

The magic incantation to run this job again is @k8s-bot cvm gce e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins unit/integration failed for commit 95cbd352e1f88fcb7bc5a27752079b412ea33d66. Full PR test history.

The magic incantation to run this job again is @k8s-bot unit test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@xialonglee xialonglee force-pushed the mention-overflows branch 4 times, most recently from 967e7ef to c630351 Compare November 10, 2016 15:53
@k8s-ci-robot
Copy link
Contributor

Jenkins GCE etcd3 e2e failed for commit c630351c686f7efdbcd39a67c97dbf9837592c35. Full PR test history.

The magic incantation to run this job again is @k8s-bot gce etcd3 e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins verification failed for commit c630351c686f7efdbcd39a67c97dbf9837592c35. Full PR test history.

The magic incantation to run this job again is @k8s-bot verify test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-github-robot k8s-github-robot added the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Nov 11, 2016
@apelisse apelisse removed the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Nov 11, 2016
@brendandburns
Copy link
Contributor

You need to run ./hack/update-bazel.sh

@xialonglee
Copy link
Contributor Author

@brendandburns
Thanks!
Firtst, it's my carelessness not to read the doc of ‘’pull-requests.md" carafully and i found the problem that I imported a new vendor pacakage 'glog' and seems some deps of relavent BUILD file not update so that the verify test failed.
I am now not familiar with the mechanism of the verify test and i tried to edit some BUILD file such as ./vendor/BUILD and intstr/BUILD, but it didn't work. If yout don't mind, could you give some help for this?or i also can figure out that eventually .
I will pass the verify procedure locally before updating my branch.

@brendandburns
Copy link
Contributor

You shouldn't need to manually edit BUILD files, just run the ./hack/update-bazel.sh script and it should update things.

--brendan

@xialonglee
Copy link
Contributor Author

@brendandburns
I got it. Thanks a lot!
I ran the script and pass the bazel verify, i think it's ok to go this time.
Thanks again.

@xialonglee
Copy link
Contributor Author

xialonglee commented Nov 14, 2016 •
edited
Loading

hi @brendandburns , all tests had passed, could you take a look at this?
Thanks.

@brendandburns brendandburns added release-note Denotes a PR that will be considered when it comes time to generate release notes. lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed release-note-label-needed labels Nov 15, 2016
@brendandburns brendandburns added this to the v1.6 milestone Nov 15, 2016
@brendandburns
Copy link
Contributor

LGTM. Thanks for the PR!

@k8s-ci-robot
Copy link
Contributor

Jenkins GCI GCE e2e failed for commit 06f138a. Full PR test history.

The magic incantation to run this job again is @k8s-bot gci gce e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@xialonglee
Copy link
Contributor Author

@k8s-bot gci gce e2e test this

@k8s-ci-robot
Copy link
Contributor

@xialonglee: you can't request testing unless you are a kubernetes member.

In response to this comment:

@k8s-bot gci gce e2e test this

If you have questions or suggestions related to this bot's behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 1d0b9e5 into kubernetes:master Nov 29, 2016
@xialonglee xialonglee deleted the mention-overflows branch November 30, 2016 00:52
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@brendandburns brendandburns

Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
v1.6
Development

Successfully merging this pull request may close these issues.
8 participants
@xialonglee @k8s-ci-robot @dims @brendandburns @k8s-github-robot @apelisse @googlebot @wojtek-t