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.

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

Python3 doesn't have random.jumpahead #15

Merged
merged 1 commit into from
Aug 20, 2016

Conversation

cybergrind
Copy link
Contributor

Replace it with seed call

@@ -17,5 +16,5 @@ def generate_id():
pid = os.getpid()
if (_fork_guard_pid == 0) or (_fork_guard_pid != pid):
_fork_guard_pid = pid
guid_rng.jumpahead(int(1000000 * time.time()) ^ pid)
guid_rng.seed(random.randrange(0, 0xFFFFFFFF))
Copy link
Member

Choose a reason for hiding this comment

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

wouldn't this produce identical result across all forks? The XOR with pid was adding uniqueness to each fork.

Copy link
Contributor

Choose a reason for hiding this comment

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

Seed is fine, but the arg must incorporate pid (and should incorporate time IMO)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense

@yurishkuro
Copy link
Member

If we want this change so that it works with python3, we should test it with 3. Can you add it to .travis.yaml matrix?

@cybergrind cybergrind force-pushed the master branch 2 times, most recently from 30e534a to 2ceba0d Compare August 20, 2016 16:11
@cybergrind
Copy link
Contributor Author

I've added time + pid into seed.

Unfortunately protobuf tests aren't fine on python 3 due discrepancies in implementation of bytearray support
https://travis-ci.org/opentracing/basictracer-python/jobs/153806259
Looks like protocolbuffers/protobuf#1173 caused it

I believe we can get rid of jumpahead now just to not break current applications that aren't rely on protobuf serialization and fix tests in separate branch. Btw it isn't look like it going to be fixed in nearest future in protobuf repo.

@yurishkuro
Copy link
Member

OK fair enough, I will accept it. Thanks for checking against 3.x, I booked it as a separate issue maybe someone will care to fix: #16

@yurishkuro yurishkuro merged commit 8474027 into opentracing:master Aug 20, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants