Skip to content

Conversation

mikeproeng37
Copy link
Contributor

Summary

  • Introduces a composite experiment decision service that wraps around and applies audience target and bucketing decision services in that order.
  • Makes the bucketer an interface so we can mock out in unit tests and renames the existing bucketer to MurmurhashBucketer

Tests

  • Unit tests

Copy link
Contributor

@aliabbasrizvi aliabbasrizvi left a comment

Choose a reason for hiding this comment

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

Looks good to me.

"github.com/twmb/murmur3"
)

var logger = logging.GetLogger("ExperimentBucketer")
var maxHashValue = float32(math.Pow(2, 32))

// DefaultHashSeed is the hash seed to use for murmurhash
const DefaultHashSeed = 1
const maxTrafficValue = 10000
Copy link
Contributor

Choose a reason for hiding this comment

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

nit. Technically this is maxBucketValue

"github.com/twmb/murmur3"
)

var logger = logging.GetLogger("ExperimentBucketer")
var maxHashValue = float32(math.Pow(2, 32))

// DefaultHashSeed is the hash seed to use for murmurhash
const DefaultHashSeed = 1
Copy link
Contributor

Choose a reason for hiding this comment

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

nit. Curious about the syntax here. This name begins with capital letter and the next name begins with small letter. Are we adhering to the standards described here: https://golang.org/doc/effective_go.html#names?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, capitalized means we are making it an exported value in this package. I made it public because we reference it from the experiment_bucketer_service which uses it to create an instance of the MurmurhashBucketer. We could just not take in the hash seed in that factory method and default it to this constant. I have no strong opinions about it.

bucketValue := b.generateBucketValue(bucketKey)
// logger.Info()
Copy link
Contributor

Choose a reason for hiding this comment

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

Left from debugging?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah

@mikeproeng37 mikeproeng37 merged commit 1de7815 into go-alpha Jul 9, 2019
@mikeproeng37 mikeproeng37 deleted the mng/composite-exp-service branch September 9, 2019 23:21
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.

3 participants