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

Allow concurrent, re-creatable usage #44

Merged
merged 6 commits into from
Apr 16, 2019
Merged

Conversation

trabetti
Copy link
Contributor

Related to issue #43.
To enable concurrent and re-creatable usage, I have added a UuidSource, similar to the source in math/rand.
It holds its own random number generator (the rander).
I have added only the methods that seemed relevant: SetRand(), NewRandom() (version 4) and New(). From what I saw, other methods do not use random number generator (except the clock sequence but this can't be recreated anyway).

@trabetti
Copy link
Contributor Author

Build failed. I used math/rand.Rand as an io.Reader and it failed saying it does not implement the io.Reader interface. It does not fail in my env, I think it is because of the Go version in the travis build is old. Although I see that math/rand.Rand implements io.Reader since 2015. Anyway, it is just for the tests, I will use a string reader instead.

@trabetti
Copy link
Contributor Author

@pborman


func TestUuidSources(t *testing.T) {

myString, _ := regen.Generate("[a-zA-Z]{1000}")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Rather than depend on another external package (creating another dependency) why not just make a very simple reader that returns a deterministic set based on some seed. For example:

type reader struct {
	n int
	c int
}

func (r *reader) Read(buf []byte) (int, error) {
	// Make an uninitialized reader just return sequential bytes.
	 if r.n == 0 {
		r.n = 1
	}
	c, n := r.c, r.n
	for i := range buf {
		c = (c + n) * n
		buf[i] = byte(c)
	}
	r.c = c
	return len(buf), nil
}

uuid_source.go Outdated
// Calling NewSource with nil sets the random number generator to a default
// generator.
func NewSource(r io.Reader) UuidSource {
var uuidSource UuidSource
Copy link
Collaborator

Choose a reason for hiding this comment

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

return Source{render: r}

No reason to special case nil. They can pass in rand.Reader if they want.

uuid_source.go Outdated
//
// It is useful when a process need its own random number generator,
// e.g. when running some processes concurrently.
type UuidSource struct {
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be UUIDSource, but if you are mimicking math/rand then just call it Source.

One issue with this setup it you still can only have a single rander function. Rather than add all of this complexity, it might be easier to simply create one new function, say NewRandomFromReader (I don't really like that name, it is a bit of a mouthful, but I don't expect it to be used often at all).

func NewRandomFromReadr(r io.Reader) (UUID, error)

Now an external package can create this type if they want.

type Source struct {
        r io.Reader
}

func NewUUIDSource(r io.Reader) Source {
        return Source{r: r}
}

func (s Source) NewRandom() (uuid.UUID, error) {
        return uuid.NewRandomFromReader(s.r)
}

That will keep the uuid package cleaner.

Copy link
Contributor Author

@trabetti trabetti Mar 13, 2019

Choose a reason for hiding this comment

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

@pborman , Thanks for looking into this PR!
Let me ask to clarify - are you suggesting here that instead of everything in the uuid_source file, we just add the func NewRandomFromReadr(r io.Reader) (UUID, error) function to version4.go and let the user build the structs around it if they need? (it is actually the func newRandom(r io.Reader) (UUID, error) that I added there). I am not clear, as you are also commenting on the content of the uuid_source files.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, that is exactly correct. And yes, the NewRandomFromReader is essentially the newRandom function you added. The uuid_source files would no longer be needed, although a simple test showing that NewRandomFromReader did in fact use the supplied reader would probably be a good addition. This could be very simple where you have a bytes.NewReader that has 16 bytes of data and the first NewRandomFromReader gives you the UUID you expect and the second NewRandomFromReader returns an error.

You might want to consider putting a package in GitHub that has the functionality of your uuid_source file so others can also use it if they need that sort of functionality.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, Got it.

You might want to consider putting a package in GitHub that has the functionality of your uuid_source file so others can also use it if they need that sort of functionality.

A package for using it seems too much for just few lines of code. Maybe I'll just add an example?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@trabetti
Copy link
Contributor Author

@pborman , do you still intend to merge this? Thank you!

Copy link
Collaborator

@pborman pborman left a comment

Choose a reason for hiding this comment

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

LGTM

@pborman pborman merged commit c2e93f3 into google:master Apr 16, 2019
@pborman
Copy link
Collaborator

pborman commented Apr 16, 2019

Sorry for the delay. Thank you for the contribution.

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.

2 participants