You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by GitBox <gi...@apache.org> on 2020/11/03 13:33:35 UTC
[GitHub] [beam] tszerszen opened a new pull request #13245: [BEAM-11075] - GO SDK - Synthetic source HotKeys and HotKeysFraction
tszerszen opened a new pull request #13245:
URL: https://github.com/apache/beam/pull/13245
Add support for HotKeys and HotKeysFraction when generating Synthetic Source in Beam GO SDK
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] tszerszen commented on a change in pull request #13245: [BEAM-11075] - GO SDK - Synthetic source HotKeys and HotKeysFraction
Posted by GitBox <gi...@apache.org>.
tszerszen commented on a change in pull request #13245:
URL: https://github.com/apache/beam/pull/13245#discussion_r517833428
##########
File path: sdks/go/pkg/beam/io/synthetic/source.go
##########
@@ -26,13 +26,12 @@ import (
"bytes"
"encoding/json"
"fmt"
+ "github.com/apache/beam/sdks/go/pkg/beam"
Review comment:
Thank you for your review, I reorganised the imports that way.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] kamilwu commented on a change in pull request #13245: [BEAM-11075] - GO SDK - Synthetic source HotKeys and HotKeysFraction
Posted by GitBox <gi...@apache.org>.
kamilwu commented on a change in pull request #13245:
URL: https://github.com/apache/beam/pull/13245#discussion_r517263905
##########
File path: sdks/go/pkg/beam/io/synthetic/source_test.go
##########
@@ -168,62 +169,48 @@ func TestSourceConfig_BuildFromJSON(t *testing.T) {
// for a synthetic source works correctly.
func TestSourceConfigBuilder_NumHotKeys(t *testing.T) {
tests := []struct {
- elms int
- hotKeys int
+ elms int
+ hotKeys int
+ hotKeyFraction float64
}{
- {elms: 15, hotKeys: 2},
- {elms: 30, hotKeys: 10},
- {elms: 50, hotKeys: 25},
+ {elms: 15, hotKeys: 2, hotKeyFraction: 1.0},
+ {elms: 30, hotKeys: 10, hotKeyFraction: 1.0},
+ {elms: 50, hotKeys: 25, hotKeyFraction: 1.0},
+ {elms: 30, hotKeys: 10, hotKeyFraction: 0.5},
+ {elms: 50, hotKeys: 25, hotKeyFraction: 0.75},
}
for _, test := range tests {
test := test
t.Run(fmt.Sprintf("(elm = %v)", test.hotKeys), func(t *testing.T) {
dfn := sourceFn{}
cfg := DefaultSourceConfig()
cfg.NumElements(test.elms)
- cfg.HotKeyFraction(1.0)
+ cfg.HotKeyFraction(test.hotKeyFraction)
cfg.NumHotKeys(test.hotKeys)
keys, _, err := simulateSourceFn(t, &dfn, cfg.Build())
if err != nil {
t.Errorf("Failure processing sourceFn: %v", err)
}
- type val struct {
- num int
- isHotKey bool
- }
-
- m := make(map[string]val)
+ m := make(map[string]int)
for i := 0; i < len(keys); i++ {
Review comment:
Do you really need nested iterations? If I'm not mistaken, this code would be just fine:
```
for _, key := range keys {
encoded := hex.EncodeToString(key)
m[encoded]++
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] tszerszen commented on a change in pull request #13245: [BEAM-11075] - GO SDK - Synthetic source HotKeys and HotKeysFraction
Posted by GitBox <gi...@apache.org>.
tszerszen commented on a change in pull request #13245:
URL: https://github.com/apache/beam/pull/13245#discussion_r517254914
##########
File path: sdks/go/pkg/beam/io/synthetic/source_test.go
##########
@@ -162,6 +164,83 @@ func TestSourceConfig_BuildFromJSON(t *testing.T) {
}
}
+// TestSourceConfig_NumHotKeys tests that setting the number of hot keys
+// for a synthetic source works correctly.
+func TestSourceConfigBuilder_NumHotKeys(t *testing.T) {
+ tests := []struct {
+ elms int
+ hotKeys int
+ }{
+ {elms: 15, hotKeys: 2},
+ {elms: 30, hotKeys: 10},
+ {elms: 50, hotKeys: 25},
+ }
+ for _, test := range tests {
+ test := test
+ t.Run(fmt.Sprintf("(elm = %v)", test.hotKeys), func(t *testing.T) {
+ dfn := sourceFn{}
+ cfg := DefaultSourceConfig()
+ cfg.NumElements(test.elms)
+ cfg.HotKeyFraction(1.0)
+ cfg.NumHotKeys(test.hotKeys)
+
+ keys, _, err := simulateSourceFn(t, &dfn, cfg.Build())
+ if err != nil {
+ t.Errorf("Failure processing sourceFn: %v", err)
+ }
+
+ type val struct {
+ num int
+ isHotKey bool
+ }
+
+ m := make(map[string]val)
Review comment:
Thank you for help!
@kamilwu wrote:
```golang
counter := make(map[byte]int) // no need to convert to string
```
I think it should be byte array
```golang
counter := make(map[[]byte]int)
```
And you can't use byte array in map.
```golang
for _, key := range keys {
counter[key]++
}
```
You're right. In the last commit I removed the struct and used direct counter, also I added support for testing HotKeyFraction. However I'm a bit worried that testing HotKeyFraction could be Flaky, what do you think?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] kamilwu merged pull request #13245: [BEAM-11075] Go SDK's synthetic source supports hot keys generation
Posted by GitBox <gi...@apache.org>.
kamilwu merged pull request #13245:
URL: https://github.com/apache/beam/pull/13245
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] tszerszen commented on a change in pull request #13245: [BEAM-11075] - GO SDK - Synthetic source HotKeys and HotKeysFraction
Posted by GitBox <gi...@apache.org>.
tszerszen commented on a change in pull request #13245:
URL: https://github.com/apache/beam/pull/13245#discussion_r517835487
##########
File path: sdks/go/pkg/beam/io/synthetic/source.go
##########
@@ -131,8 +130,19 @@ func (fn *sourceFn) ProcessElement(rt *sdf.LockRTracker, config SourceConfig, em
for i := rt.GetRestriction().(offsetrange.Restriction).Start; rt.TryClaim(i) == true; i++ {
key := make([]byte, config.KeySize)
val := make([]byte, config.ValueSize)
- if _, err := fn.rng.Read(key); err != nil {
- return err
+ generator := sourceFn{}
+ generator.rng = rand.New(rand.NewSource(i))
Review comment:
@youngoli thank you for your feedback. Since I have the most experience with Python, when I was implementing it I was following the way it was implemented in Python.
In Python generator is seed with each index, therefore I did the same in Go, and I seed the generator with a NewSource on each iteration. There is no salt in config in other SDKs implementation and the idea behind it was: To be consistent with other SDKs to be able to compare them.
```python
def _gen_kv_pair(self, generator, index):
generator.seed(index)
rand = generator.random_sample()
# Determines whether to generate hot key or not.
if rand < self._hot_key_fraction:
# Generate hot key.
# An integer is randomly selected from the range [0, numHotKeys-1]
# with equal probability.
generator_hot = Generator(index % self._num_hot_keys)
bytes_ = generator_hot.bytes(self._key_size), generator.bytes(
self._value_size)
else:
bytes_ = generator.bytes(self.element_size)
bytes_ = bytes_[:self._key_size], bytes_[self._key_size:]
return bytes_
def read(self, range_tracker):
index = range_tracker.start_position()
generator = Generator()
while range_tracker.try_claim(index):
time.sleep(self._sleep_per_input_record_sec)
yield self._gen_kv_pair(generator, index)
index += 1
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] tszerszen commented on a change in pull request #13245: [BEAM-11075] - GO SDK - Synthetic source HotKeys and HotKeysFraction
Posted by GitBox <gi...@apache.org>.
tszerszen commented on a change in pull request #13245:
URL: https://github.com/apache/beam/pull/13245#discussion_r517289194
##########
File path: sdks/go/pkg/beam/io/synthetic/source_test.go
##########
@@ -162,6 +164,83 @@ func TestSourceConfig_BuildFromJSON(t *testing.T) {
}
}
+// TestSourceConfig_NumHotKeys tests that setting the number of hot keys
+// for a synthetic source works correctly.
+func TestSourceConfigBuilder_NumHotKeys(t *testing.T) {
+ tests := []struct {
+ elms int
+ hotKeys int
+ }{
+ {elms: 15, hotKeys: 2},
+ {elms: 30, hotKeys: 10},
+ {elms: 50, hotKeys: 25},
+ }
+ for _, test := range tests {
+ test := test
+ t.Run(fmt.Sprintf("(elm = %v)", test.hotKeys), func(t *testing.T) {
+ dfn := sourceFn{}
+ cfg := DefaultSourceConfig()
+ cfg.NumElements(test.elms)
+ cfg.HotKeyFraction(1.0)
+ cfg.NumHotKeys(test.hotKeys)
+
+ keys, _, err := simulateSourceFn(t, &dfn, cfg.Build())
+ if err != nil {
+ t.Errorf("Failure processing sourceFn: %v", err)
+ }
+
+ type val struct {
+ num int
+ isHotKey bool
+ }
+
+ m := make(map[string]val)
Review comment:
@kamilwu wrote:
```Let's set HotKeyFraction to 1.0. Testing only NumHotKeys is fine for me.```
I commented test cases with HotKeyFraction < 1.0 and left the logic behind it. Is that ok?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] tszerszen commented on a change in pull request #13245: [BEAM-11075] - GO SDK - Synthetic source HotKeys and HotKeysFraction
Posted by GitBox <gi...@apache.org>.
tszerszen commented on a change in pull request #13245:
URL: https://github.com/apache/beam/pull/13245#discussion_r518565147
##########
File path: sdks/go/pkg/beam/io/synthetic/source.go
##########
@@ -131,8 +130,19 @@ func (fn *sourceFn) ProcessElement(rt *sdf.LockRTracker, config SourceConfig, em
for i := rt.GetRestriction().(offsetrange.Restriction).Start; rt.TryClaim(i) == true; i++ {
key := make([]byte, config.KeySize)
val := make([]byte, config.ValueSize)
- if _, err := fn.rng.Read(key); err != nil {
- return err
+ generator := sourceFn{}
+ generator.rng = rand.New(rand.NewSource(i))
Review comment:
@youngoli Thank you, I pushed the changes you suggested.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] tszerszen commented on a change in pull request #13245: [BEAM-11075] - GO SDK - Synthetic source HotKeys and HotKeysFraction
Posted by GitBox <gi...@apache.org>.
tszerszen commented on a change in pull request #13245:
URL: https://github.com/apache/beam/pull/13245#discussion_r517296798
##########
File path: sdks/go/pkg/beam/io/synthetic/source_test.go
##########
@@ -175,8 +175,8 @@ func TestSourceConfigBuilder_NumHotKeys(t *testing.T) {
{elms: 15, hotKeys: 2, hotKeyFraction: 1.0},
{elms: 30, hotKeys: 10, hotKeyFraction: 1.0},
{elms: 50, hotKeys: 25, hotKeyFraction: 1.0},
- {elms: 30, hotKeys: 10, hotKeyFraction: 0.5},
- {elms: 50, hotKeys: 25, hotKeyFraction: 0.75},
+ // {elms: 30, hotKeys: 10, hotKeyFraction: 0.5}, tests for hotKeyFraction < 1.0 could be Flaky
Review comment:
I got it, I removed Hot Key Fraction testing completely.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] tszerszen commented on a change in pull request #13245: [BEAM-11075] - GO SDK - Synthetic source HotKeys and HotKeysFraction
Posted by GitBox <gi...@apache.org>.
tszerszen commented on a change in pull request #13245:
URL: https://github.com/apache/beam/pull/13245#discussion_r516502298
##########
File path: sdks/go/pkg/beam/io/synthetic/source.go
##########
@@ -165,10 +175,12 @@ type SourceConfigBuilder struct {
func DefaultSourceConfig() *SourceConfigBuilder {
return &SourceConfigBuilder{
cfg: SourceConfig{
- NumElements: 1, // 0 is invalid (drops elements).
- InitialSplits: 1, // 0 is invalid (drops elements).
- KeySize: 8, // 0 is invalid (drops elements).
- ValueSize: 8, // 0 is invalid (drops elements).
+ NumElements: 1, // 0 is invalid (drops elements).
+ InitialSplits: 1, // 0 is invalid (drops elements).
+ KeySize: 8, // 0 is invalid (drops elements).
+ ValueSize: 8, // 0 is invalid (drops elements).
+ HotKeys: 2, // 0 is invalid (drops elements).
Review comment:
```
NumElements: 1, // 0 is invalid (drops elements).
InitialSplits: 1, // 0 is invalid (drops elements).
KeySize: 8, // 0 is invalid (drops elements).
ValueSize: 8, // 0 is invalid (drops elements).
NumHotKeys: 0,
HotKeyFraction: 0,
```
##########
File path: sdks/go/pkg/beam/io/synthetic/source.go
##########
@@ -165,10 +175,12 @@ type SourceConfigBuilder struct {
func DefaultSourceConfig() *SourceConfigBuilder {
return &SourceConfigBuilder{
cfg: SourceConfig{
- NumElements: 1, // 0 is invalid (drops elements).
- InitialSplits: 1, // 0 is invalid (drops elements).
- KeySize: 8, // 0 is invalid (drops elements).
- ValueSize: 8, // 0 is invalid (drops elements).
+ NumElements: 1, // 0 is invalid (drops elements).
+ InitialSplits: 1, // 0 is invalid (drops elements).
+ KeySize: 8, // 0 is invalid (drops elements).
+ ValueSize: 8, // 0 is invalid (drops elements).
+ HotKeys: 2, // 0 is invalid (drops elements).
Review comment:
```
NumElements: 1, // 0 is invalid (drops elements).
InitialSplits: 1, // 0 is invalid (drops elements).
KeySize: 8, // 0 is invalid (drops elements).
ValueSize: 8, // 0 is invalid (drops elements).
NumHotKeys: 0,
HotKeyFraction: 0,
```
##########
File path: sdks/go/pkg/beam/io/synthetic/source.go
##########
@@ -262,8 +281,10 @@ func (b *SourceConfigBuilder) BuildFromJSON(jsonData []byte) SourceConfig {
// synthetic source. It should be created via a SourceConfigBuilder, not by
// directly initializing it (the fields are public to allow encoding).
type SourceConfig struct {
- NumElements int `json:"num_records"`
- InitialSplits int `json:"initial_splits"`
- KeySize int `json:"key_size"`
- ValueSize int `json:"value_size"`
+ NumElements int `json:"num_records"`
+ InitialSplits int `json:"initial_splits"`
+ HotKeys int `json:"hot_keys"`
Review comment:
```
NumHotKeys int `json:"num_hot_keys"`
HotKeyFraction float64 `json:"hot_key_fraction"`
```
##########
File path: sdks/go/pkg/beam/io/synthetic/source.go
##########
@@ -262,8 +281,10 @@ func (b *SourceConfigBuilder) BuildFromJSON(jsonData []byte) SourceConfig {
// synthetic source. It should be created via a SourceConfigBuilder, not by
// directly initializing it (the fields are public to allow encoding).
type SourceConfig struct {
- NumElements int `json:"num_records"`
- InitialSplits int `json:"initial_splits"`
- KeySize int `json:"key_size"`
- ValueSize int `json:"value_size"`
+ NumElements int `json:"num_records"`
+ InitialSplits int `json:"initial_splits"`
+ HotKeys int `json:"hot_keys"`
Review comment:
```
// NumHotKeys determines the number of keys with the same value among
// generated keys.
//
// Valid values are in the range of [0, ...] and the default value is 0.
func (b *SourceConfigBuilder) NumHotKeys(val int) *SourceConfigBuilder {
b.cfg.NumHotKeys = val
return b
}
// HotKeyFraction determines the value of hot key fraction.
//
// Valid values are floating point numbers from 0 to 1.
func (b *SourceConfigBuilder) HotKeyFraction(val float64) *SourceConfigBuilder {
b.cfg.HotKeyFraction = val
return b
}
```
##########
File path: sdks/go/pkg/beam/io/synthetic/source.go
##########
@@ -235,6 +247,12 @@ func (b *SourceConfigBuilder) Build() SourceConfig {
if b.cfg.ValueSize <= 0 {
panic(fmt.Sprintf("SourceConfig.ValueSize must be >= 1. Got: %v", b.cfg.ValueSize))
}
+ if b.cfg.HotKeys <= 0 {
+ panic(fmt.Sprintf("SourceConfig.HotKeys must be >= 1. Got: %v", b.cfg.HotKeys))
+ }
+ if b.cfg.HotKeyFraction <= 0 {
+ panic(fmt.Sprintf("SourceConfig.HotKeyFraction must be >= 1. Got: %v", b.cfg.HotKeyFraction))
Review comment:
```
if b.cfg.HotKeyFraction < 0 || b.cfg.HotKeyFraction > 1 {
panic(fmt.Sprintf("SourceConfig.HotKeyFraction must be a floating point number from 0 and 1. Got: %v", b.cfg.NumHotKeys))
}
```
##########
File path: sdks/go/pkg/beam/io/synthetic/source.go
##########
@@ -246,7 +264,8 @@ func (b *SourceConfigBuilder) Build() SourceConfig {
// {
// "num_records": 5,
// "key_size": 5,
-// "value_size": 5
+// "value_size": 5,
+// "num_keys": 5,
Review comment:
```
// {
// "num_records": 5,
// "key_size": 5,
// "value_size": 5
// "value_size": 5,
// "num_hot_keys": 5,
// }
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] tszerszen commented on a change in pull request #13245: [BEAM-11075] - GO SDK - Synthetic source HotKeys and HotKeysFraction
Posted by GitBox <gi...@apache.org>.
tszerszen commented on a change in pull request #13245:
URL: https://github.com/apache/beam/pull/13245#discussion_r517833781
##########
File path: sdks/go/pkg/beam/io/synthetic/source.go
##########
@@ -131,8 +130,19 @@ func (fn *sourceFn) ProcessElement(rt *sdf.LockRTracker, config SourceConfig, em
for i := rt.GetRestriction().(offsetrange.Restriction).Start; rt.TryClaim(i) == true; i++ {
key := make([]byte, config.KeySize)
val := make([]byte, config.ValueSize)
- if _, err := fn.rng.Read(key); err != nil {
- return err
+ generator := sourceFn{}
+ generator.rng = rand.New(rand.NewSource(i))
+ randomSample := generator.rng.Float64()
Review comment:
You're right I fixed it, thank you.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] youngoli commented on a change in pull request #13245: [BEAM-11075] - GO SDK - Synthetic source HotKeys and HotKeysFraction
Posted by GitBox <gi...@apache.org>.
youngoli commented on a change in pull request #13245:
URL: https://github.com/apache/beam/pull/13245#discussion_r518452815
##########
File path: sdks/go/pkg/beam/io/synthetic/source.go
##########
@@ -131,8 +130,19 @@ func (fn *sourceFn) ProcessElement(rt *sdf.LockRTracker, config SourceConfig, em
for i := rt.GetRestriction().(offsetrange.Restriction).Start; rt.TryClaim(i) == true; i++ {
key := make([]byte, config.KeySize)
val := make([]byte, config.ValueSize)
- if _, err := fn.rng.Read(key); err != nil {
- return err
+ generator := sourceFn{}
+ generator.rng = rand.New(rand.NewSource(i))
Review comment:
Hm... I have objections with the way that one is coded too, but that's something for me to discuss with the original author. It's definitely reasonable to match the Python implementation.
In that case, my only remaining advice is, instead of creating a new Rand object with a new source each time you want to reseed (in other words, in each position), create the Rand objects in the beginning of `ProcessElement` and then call [Rand.Seed](https://golang.org/pkg/math/rand/#Rand.Seed) when you want to reseed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] tszerszen commented on a change in pull request #13245: [BEAM-11075] - GO SDK - Synthetic source HotKeys and HotKeysFraction
Posted by GitBox <gi...@apache.org>.
tszerszen commented on a change in pull request #13245:
URL: https://github.com/apache/beam/pull/13245#discussion_r517832943
##########
File path: sdks/go/pkg/beam/io/synthetic/source_test.go
##########
@@ -162,6 +163,59 @@ func TestSourceConfig_BuildFromJSON(t *testing.T) {
}
}
+// TestSourceConfig_NumHotKeys tests that setting the number of hot keys
+// for a synthetic source works correctly.
+func TestSourceConfigBuilder_NumHotKeys(t *testing.T) {
+ tests := []struct {
+ elms int
+ hotKeys int
+ }{
+ {elms: 15, hotKeys: 2},
+ {elms: 30, hotKeys: 10},
+ {elms: 50, hotKeys: 25},
+ }
+ for _, test := range tests {
+ test := test
+ t.Run(fmt.Sprintf("(elm = %v)", test.hotKeys), func(t *testing.T) {
+ dfn := sourceFn{}
+ cfg := DefaultSourceConfig()
+ cfg.NumElements(test.elms)
+ cfg.HotKeyFraction(1.0)
+ cfg.NumHotKeys(test.hotKeys)
+
+ keys, _, err := simulateSourceFn(t, &dfn, cfg.Build())
+ if err != nil {
+ t.Errorf("Failure processing sourceFn: %v", err)
+ }
+
+ m := make(map[string]int)
+ for _, key := range keys {
+ encoded := hex.EncodeToString(key)
+ m[encoded]++
+ }
+
+ numOfHotKeys := 0
+ numOfAllKeys := 0
+ for _, element := range m {
+ numOfAllKeys += element
+ if element > 1 {
+ numOfHotKeys += 1
+ }
+ }
+
+ if numOfAllKeys != test.elms {
Review comment:
Thank you, I removed this check.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] tszerszen commented on pull request #13245: [BEAM-11075] - GO SDK - Synthetic source HotKeys and HotKeysFraction
Posted by GitBox <gi...@apache.org>.
tszerszen commented on pull request #13245:
URL: https://github.com/apache/beam/pull/13245#issuecomment-720495521
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] kamilwu commented on a change in pull request #13245: [BEAM-11075] - GO SDK - Synthetic source HotKeys and HotKeysFraction
Posted by GitBox <gi...@apache.org>.
kamilwu commented on a change in pull request #13245:
URL: https://github.com/apache/beam/pull/13245#discussion_r517294852
##########
File path: sdks/go/pkg/beam/io/synthetic/source_test.go
##########
@@ -175,8 +175,8 @@ func TestSourceConfigBuilder_NumHotKeys(t *testing.T) {
{elms: 15, hotKeys: 2, hotKeyFraction: 1.0},
{elms: 30, hotKeys: 10, hotKeyFraction: 1.0},
{elms: 50, hotKeys: 25, hotKeyFraction: 1.0},
- {elms: 30, hotKeys: 10, hotKeyFraction: 0.5},
- {elms: 50, hotKeys: 25, hotKeyFraction: 0.75},
+ // {elms: 30, hotKeys: 10, hotKeyFraction: 0.5}, tests for hotKeyFraction < 1.0 could be Flaky
Review comment:
In general, we shouldn't commit commented out code. That being said, there are two solutions:
1) remove this completely,
2) if you feel that `hotKeyFraction` should be tested, but not necessarily now for whatever reason, create a // TODO(BEAM-xxx) and describe what should be done here.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] kamilwu commented on a change in pull request #13245: [BEAM-11075] - GO SDK - Synthetic source HotKeys and HotKeysFraction
Posted by GitBox <gi...@apache.org>.
kamilwu commented on a change in pull request #13245:
URL: https://github.com/apache/beam/pull/13245#discussion_r517285596
##########
File path: sdks/go/pkg/beam/io/synthetic/source_test.go
##########
@@ -162,6 +164,83 @@ func TestSourceConfig_BuildFromJSON(t *testing.T) {
}
}
+// TestSourceConfig_NumHotKeys tests that setting the number of hot keys
+// for a synthetic source works correctly.
+func TestSourceConfigBuilder_NumHotKeys(t *testing.T) {
+ tests := []struct {
+ elms int
+ hotKeys int
+ }{
+ {elms: 15, hotKeys: 2},
+ {elms: 30, hotKeys: 10},
+ {elms: 50, hotKeys: 25},
+ }
+ for _, test := range tests {
+ test := test
+ t.Run(fmt.Sprintf("(elm = %v)", test.hotKeys), func(t *testing.T) {
+ dfn := sourceFn{}
+ cfg := DefaultSourceConfig()
+ cfg.NumElements(test.elms)
+ cfg.HotKeyFraction(1.0)
+ cfg.NumHotKeys(test.hotKeys)
+
+ keys, _, err := simulateSourceFn(t, &dfn, cfg.Build())
+ if err != nil {
+ t.Errorf("Failure processing sourceFn: %v", err)
+ }
+
+ type val struct {
+ num int
+ isHotKey bool
+ }
+
+ m := make(map[string]val)
Review comment:
Yes, you're right, it should be a byte array. We can't use arrays/slices as a key, but we can convert `[]byte` to `string`. You've already done this, so this is good!
> However I'm a bit worried that testing HotKeyFraction could be Flaky, what do you think?
Yes, there is some probability that this would happen unless we mock the generator. Let's set `HotKeyFraction` to 1.0. Testing only `NumHotKeys` is fine for me.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] tszerszen commented on a change in pull request #13245: [BEAM-11075] - GO SDK - Synthetic source HotKeys and HotKeysFraction
Posted by GitBox <gi...@apache.org>.
tszerszen commented on a change in pull request #13245:
URL: https://github.com/apache/beam/pull/13245#discussion_r517268738
##########
File path: sdks/go/pkg/beam/io/synthetic/source_test.go
##########
@@ -168,62 +169,48 @@ func TestSourceConfig_BuildFromJSON(t *testing.T) {
// for a synthetic source works correctly.
func TestSourceConfigBuilder_NumHotKeys(t *testing.T) {
tests := []struct {
- elms int
- hotKeys int
+ elms int
+ hotKeys int
+ hotKeyFraction float64
}{
- {elms: 15, hotKeys: 2},
- {elms: 30, hotKeys: 10},
- {elms: 50, hotKeys: 25},
+ {elms: 15, hotKeys: 2, hotKeyFraction: 1.0},
+ {elms: 30, hotKeys: 10, hotKeyFraction: 1.0},
+ {elms: 50, hotKeys: 25, hotKeyFraction: 1.0},
+ {elms: 30, hotKeys: 10, hotKeyFraction: 0.5},
+ {elms: 50, hotKeys: 25, hotKeyFraction: 0.75},
}
for _, test := range tests {
test := test
t.Run(fmt.Sprintf("(elm = %v)", test.hotKeys), func(t *testing.T) {
dfn := sourceFn{}
cfg := DefaultSourceConfig()
cfg.NumElements(test.elms)
- cfg.HotKeyFraction(1.0)
+ cfg.HotKeyFraction(test.hotKeyFraction)
cfg.NumHotKeys(test.hotKeys)
keys, _, err := simulateSourceFn(t, &dfn, cfg.Build())
if err != nil {
t.Errorf("Failure processing sourceFn: %v", err)
}
- type val struct {
- num int
- isHotKey bool
- }
-
- m := make(map[string]val)
+ m := make(map[string]int)
for i := 0; i < len(keys); i++ {
Review comment:
You're right, thank you I pushed the fix.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] kamilwu commented on pull request #13245: [BEAM-11075] - GO SDK - Synthetic source HotKeys and HotKeysFraction
Posted by GitBox <gi...@apache.org>.
kamilwu commented on pull request #13245:
URL: https://github.com/apache/beam/pull/13245#issuecomment-721071164
Thanks. Two other notes:
1. Do you think you can add a unit test in `source_test.go` that would check your functionality?
2. Please do not squash review changes. It makes it hard for a reviewer to follow up with what has changed. Instead, create a commit like "fix: a short summary". Those commits are squashed after review is complete.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] kamilwu commented on a change in pull request #13245: [BEAM-11075] - GO SDK - Synthetic source HotKeys and HotKeysFraction
Posted by GitBox <gi...@apache.org>.
kamilwu commented on a change in pull request #13245:
URL: https://github.com/apache/beam/pull/13245#discussion_r517218424
##########
File path: sdks/go/pkg/beam/io/synthetic/source_test.go
##########
@@ -162,6 +164,83 @@ func TestSourceConfig_BuildFromJSON(t *testing.T) {
}
}
+// TestSourceConfig_NumHotKeys tests that setting the number of hot keys
+// for a synthetic source works correctly.
+func TestSourceConfigBuilder_NumHotKeys(t *testing.T) {
+ tests := []struct {
+ elms int
+ hotKeys int
+ }{
+ {elms: 15, hotKeys: 2},
+ {elms: 30, hotKeys: 10},
+ {elms: 50, hotKeys: 25},
+ }
+ for _, test := range tests {
+ test := test
+ t.Run(fmt.Sprintf("(elm = %v)", test.hotKeys), func(t *testing.T) {
+ dfn := sourceFn{}
+ cfg := DefaultSourceConfig()
+ cfg.NumElements(test.elms)
+ cfg.HotKeyFraction(1.0)
+ cfg.NumHotKeys(test.hotKeys)
+
+ keys, _, err := simulateSourceFn(t, &dfn, cfg.Build())
+ if err != nil {
+ t.Errorf("Failure processing sourceFn: %v", err)
+ }
+
+ type val struct {
+ num int
+ isHotKey bool
+ }
+
+ m := make(map[string]val)
Review comment:
I think this whole logic could be simplified. What we need to do is to count distinct elements in the `keys` slice. So it can look something like this:
```go
counter := make(map[byte]int) // no need to convert to string
for _, key := range keys {
counter[key]++
}
```
After that you can use `len(keys)` and `len(counter)` in assertions.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] tszerszen commented on pull request #13245: [BEAM-11075] - GO SDK - Synthetic source HotKeys and HotKeysFraction
Posted by GitBox <gi...@apache.org>.
tszerszen commented on pull request #13245:
URL: https://github.com/apache/beam/pull/13245#issuecomment-723062011
Run Seed Job
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] tszerszen edited a comment on pull request #13245: [BEAM-11075] - GO SDK - Synthetic source HotKeys and HotKeysFraction
Posted by GitBox <gi...@apache.org>.
tszerszen edited a comment on pull request #13245:
URL: https://github.com/apache/beam/pull/13245#issuecomment-720495521
R: @kamilwu could you please provide a review?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] kamilwu commented on a change in pull request #13245: [BEAM-11075] - GO SDK - Synthetic source HotKeys and HotKeysFraction
Posted by GitBox <gi...@apache.org>.
kamilwu commented on a change in pull request #13245:
URL: https://github.com/apache/beam/pull/13245#discussion_r517295288
##########
File path: sdks/go/pkg/beam/io/synthetic/source_test.go
##########
@@ -162,6 +164,83 @@ func TestSourceConfig_BuildFromJSON(t *testing.T) {
}
}
+// TestSourceConfig_NumHotKeys tests that setting the number of hot keys
+// for a synthetic source works correctly.
+func TestSourceConfigBuilder_NumHotKeys(t *testing.T) {
+ tests := []struct {
+ elms int
+ hotKeys int
+ }{
+ {elms: 15, hotKeys: 2},
+ {elms: 30, hotKeys: 10},
+ {elms: 50, hotKeys: 25},
+ }
+ for _, test := range tests {
+ test := test
+ t.Run(fmt.Sprintf("(elm = %v)", test.hotKeys), func(t *testing.T) {
+ dfn := sourceFn{}
+ cfg := DefaultSourceConfig()
+ cfg.NumElements(test.elms)
+ cfg.HotKeyFraction(1.0)
+ cfg.NumHotKeys(test.hotKeys)
+
+ keys, _, err := simulateSourceFn(t, &dfn, cfg.Build())
+ if err != nil {
+ t.Errorf("Failure processing sourceFn: %v", err)
+ }
+
+ type val struct {
+ num int
+ isHotKey bool
+ }
+
+ m := make(map[string]val)
Review comment:
I left the comment below.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] tszerszen commented on pull request #13245: [BEAM-11075] - GO SDK - Synthetic source HotKeys and HotKeysFraction
Posted by GitBox <gi...@apache.org>.
tszerszen commented on pull request #13245:
URL: https://github.com/apache/beam/pull/13245#issuecomment-723807325
R: @kamilwu could you handle cleanup/merging as @youngoli suggested?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] kamilwu commented on a change in pull request #13245: [BEAM-11075] - GO SDK - Synthetic source HotKeys and HotKeysFraction
Posted by GitBox <gi...@apache.org>.
kamilwu commented on a change in pull request #13245:
URL: https://github.com/apache/beam/pull/13245#discussion_r516081963
##########
File path: sdks/go/pkg/beam/io/synthetic/source.go
##########
@@ -165,10 +175,12 @@ type SourceConfigBuilder struct {
func DefaultSourceConfig() *SourceConfigBuilder {
return &SourceConfigBuilder{
cfg: SourceConfig{
- NumElements: 1, // 0 is invalid (drops elements).
- InitialSplits: 1, // 0 is invalid (drops elements).
- KeySize: 8, // 0 is invalid (drops elements).
- ValueSize: 8, // 0 is invalid (drops elements).
+ NumElements: 1, // 0 is invalid (drops elements).
+ InitialSplits: 1, // 0 is invalid (drops elements).
+ KeySize: 8, // 0 is invalid (drops elements).
+ ValueSize: 8, // 0 is invalid (drops elements).
+ HotKeys: 2, // 0 is invalid (drops elements).
Review comment:
Why "2"?
##########
File path: sdks/go/pkg/beam/io/synthetic/source.go
##########
@@ -246,7 +264,8 @@ func (b *SourceConfigBuilder) Build() SourceConfig {
// {
// "num_records": 5,
// "key_size": 5,
-// "value_size": 5
+// "value_size": 5,
+// "num_keys": 5,
Review comment:
`num_keys` -> `num_hot_keys`
##########
File path: sdks/go/pkg/beam/io/synthetic/source.go
##########
@@ -262,8 +281,10 @@ func (b *SourceConfigBuilder) BuildFromJSON(jsonData []byte) SourceConfig {
// synthetic source. It should be created via a SourceConfigBuilder, not by
// directly initializing it (the fields are public to allow encoding).
type SourceConfig struct {
- NumElements int `json:"num_records"`
- InitialSplits int `json:"initial_splits"`
- KeySize int `json:"key_size"`
- ValueSize int `json:"value_size"`
+ NumElements int `json:"num_records"`
+ InitialSplits int `json:"initial_splits"`
+ HotKeys int `json:"hot_keys"`
Review comment:
Python tests use `num_hot_keys`, let's do the same in Go. `hot_key_fraction` is correct.
##########
File path: sdks/go/pkg/beam/io/synthetic/source.go
##########
@@ -165,10 +175,12 @@ type SourceConfigBuilder struct {
func DefaultSourceConfig() *SourceConfigBuilder {
return &SourceConfigBuilder{
cfg: SourceConfig{
- NumElements: 1, // 0 is invalid (drops elements).
- InitialSplits: 1, // 0 is invalid (drops elements).
- KeySize: 8, // 0 is invalid (drops elements).
- ValueSize: 8, // 0 is invalid (drops elements).
+ NumElements: 1, // 0 is invalid (drops elements).
+ InitialSplits: 1, // 0 is invalid (drops elements).
+ KeySize: 8, // 0 is invalid (drops elements).
+ ValueSize: 8, // 0 is invalid (drops elements).
+ HotKeys: 2, // 0 is invalid (drops elements).
Review comment:
By default, there should be no hot keys at all (every key should be unique).
##########
File path: sdks/go/pkg/beam/io/synthetic/source.go
##########
@@ -235,6 +247,12 @@ func (b *SourceConfigBuilder) Build() SourceConfig {
if b.cfg.ValueSize <= 0 {
panic(fmt.Sprintf("SourceConfig.ValueSize must be >= 1. Got: %v", b.cfg.ValueSize))
}
+ if b.cfg.HotKeys <= 0 {
+ panic(fmt.Sprintf("SourceConfig.HotKeys must be >= 1. Got: %v", b.cfg.HotKeys))
+ }
+ if b.cfg.HotKeyFraction <= 0 {
+ panic(fmt.Sprintf("SourceConfig.HotKeyFraction must be >= 1. Got: %v", b.cfg.HotKeyFraction))
Review comment:
HotKeyFraction is supposed to be a floating point number from 0 to 1, right?
##########
File path: sdks/go/pkg/beam/io/synthetic/source.go
##########
@@ -262,8 +281,10 @@ func (b *SourceConfigBuilder) BuildFromJSON(jsonData []byte) SourceConfig {
// synthetic source. It should be created via a SourceConfigBuilder, not by
// directly initializing it (the fields are public to allow encoding).
type SourceConfig struct {
- NumElements int `json:"num_records"`
- InitialSplits int `json:"initial_splits"`
- KeySize int `json:"key_size"`
- ValueSize int `json:"value_size"`
+ NumElements int `json:"num_records"`
+ InitialSplits int `json:"initial_splits"`
+ HotKeys int `json:"hot_keys"`
Review comment:
I think you forgot about setters for your new variables. Please add them, because right now, we can only set them via JSON.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] youngoli commented on a change in pull request #13245: [BEAM-11075] - GO SDK - Synthetic source HotKeys and HotKeysFraction
Posted by GitBox <gi...@apache.org>.
youngoli commented on a change in pull request #13245:
URL: https://github.com/apache/beam/pull/13245#discussion_r517781915
##########
File path: sdks/go/pkg/beam/io/synthetic/source.go
##########
@@ -131,8 +130,19 @@ func (fn *sourceFn) ProcessElement(rt *sdf.LockRTracker, config SourceConfig, em
for i := rt.GetRestriction().(offsetrange.Restriction).Start; rt.TryClaim(i) == true; i++ {
key := make([]byte, config.KeySize)
val := make([]byte, config.ValueSize)
- if _, err := fn.rng.Read(key); err != nil {
- return err
+ generator := sourceFn{}
+ generator.rng = rand.New(rand.NewSource(i))
+ randomSample := generator.rng.Float64()
Review comment:
It's unnecessary to create a new sourceFn here just to use the rng field. You should be able to get the same behavior by using the rand.Rand object directly.
```suggestion
generator := rand.New(rand.NewSource(i))
randomSample := generator.Float64()
```
##########
File path: sdks/go/pkg/beam/io/synthetic/source_test.go
##########
@@ -162,6 +163,59 @@ func TestSourceConfig_BuildFromJSON(t *testing.T) {
}
}
+// TestSourceConfig_NumHotKeys tests that setting the number of hot keys
+// for a synthetic source works correctly.
+func TestSourceConfigBuilder_NumHotKeys(t *testing.T) {
+ tests := []struct {
+ elms int
+ hotKeys int
+ }{
+ {elms: 15, hotKeys: 2},
+ {elms: 30, hotKeys: 10},
+ {elms: 50, hotKeys: 25},
+ }
+ for _, test := range tests {
+ test := test
+ t.Run(fmt.Sprintf("(elm = %v)", test.hotKeys), func(t *testing.T) {
+ dfn := sourceFn{}
+ cfg := DefaultSourceConfig()
+ cfg.NumElements(test.elms)
+ cfg.HotKeyFraction(1.0)
+ cfg.NumHotKeys(test.hotKeys)
+
+ keys, _, err := simulateSourceFn(t, &dfn, cfg.Build())
+ if err != nil {
+ t.Errorf("Failure processing sourceFn: %v", err)
+ }
+
+ m := make(map[string]int)
+ for _, key := range keys {
+ encoded := hex.EncodeToString(key)
+ m[encoded]++
+ }
+
+ numOfHotKeys := 0
+ numOfAllKeys := 0
+ for _, element := range m {
+ numOfAllKeys += element
+ if element > 1 {
+ numOfHotKeys += 1
+ }
+ }
+
+ if numOfAllKeys != test.elms {
Review comment:
I don't think the numOfAllKeys check here is necessary. It essentially boils down to counting the number of elements emitted which is already done by other unit tests. The only reason I can see for this is if you suspect that enabling hot keys might cause elements to drop, when they wouldn't otherwise, but based on your implementation I don't think that's something to worry about.
##########
File path: sdks/go/pkg/beam/io/synthetic/source.go
##########
@@ -26,13 +26,12 @@ import (
"bytes"
"encoding/json"
"fmt"
+ "github.com/apache/beam/sdks/go/pkg/beam"
Review comment:
Nit: The style in the Go SDK is to separate standard Go package imports from other others. So in this case it's actually the beam/core/sdf import that should be moved to the lower group, separate from the standard Go packages.
##########
File path: sdks/go/pkg/beam/io/synthetic/source.go
##########
@@ -131,8 +130,19 @@ func (fn *sourceFn) ProcessElement(rt *sdf.LockRTracker, config SourceConfig, em
for i := rt.GetRestriction().(offsetrange.Restriction).Start; rt.TryClaim(i) == true; i++ {
key := make([]byte, config.KeySize)
val := make([]byte, config.ValueSize)
- if _, err := fn.rng.Read(key); err != nil {
- return err
+ generator := sourceFn{}
+ generator.rng = rand.New(rand.NewSource(i))
Review comment:
I would avoid making local instances of rand.Rand here unless you have a very good reason here. Is the reason you're creating new, local instances of rand.Rand and seeding them with the restriction positions because you want to get completely deterministic behavior, regardless of any splitting that might occur on the restriction?
If so: Then be aware that this implementation is going to get identical results per input (in this case, per SourceConfig). For example, if you make two SourceConfigs for 10 elements each, then both of them will seed these generators with the numbers 0-9, which can lead to strangely identical results. If you want purely deterministic output that is still different per SourceConfig, then each SourceConfig will probably need some kind of unique but deterministic salt that can be added to these numbers (example: `rand.NewSource(i + fn.salt))`).
On the other hand, if this check and the hotkey distributions don't need to be deterministic, then I would simplify a bit and just use the existing Rand object (`fn.rng`) more. Or, if you need to customize the rng for test purposes, you can add more `randWrapper` fields to the SourceFn struct. I think the only spot where you would need deterministic values in that case is for filling out the actual hotkey value, and even then I think there's room for optimizing so we don't constantly create and reseed a rand.Rand.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org