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:
[email protected]
