fix: Change options for new client #603

yevgenypats · 2023-01-11T22:25:49Z

I think I missed it in the review but the api should include an option struct rather then the ...option as otherwise it's just introduce un-necessary boilerplate for each client.

Also, I've added BackendNone as otherwise there is no way to specify that you don't want incremental syncing.

github-actions · 2023-01-11T22:30:18Z

⏱️ Benchmark results

DefaultConcurrencyDFS-2 resources/s: 10,978
DefaultConcurrencyRoundRobin-2 resources/s: 11,285
Glob-2 ns/op: 185.8
TablesWithChildrenDFS-2 resources/s: 28,111
TablesWithChildrenRoundRobin-2 resources/s: 27,491
TablesWithRateLimitingDFS-2 resources/s: 28.22
TablesWithRateLimitingRoundRobin-2 resources/s: 817.9
BufferedScanner-2 ns/op: 11.26
LogReader-2 ns/op: 34.21

yevgenypats · 2023-01-11T23:46:28Z

Now that Im thinking more about it I think we might have a problem in our incremental syncing approach.

If we specify incremental, only incremental tables should be synced as otherwise it can cause issues with destinations such as gcs.

also, on the destination side we always don't delete incremental tables which basically now we disabled an option of overwrite-delete-stale for a lot of tables (i.e let's say we just want overwrite-delete-stale without any incremental syncing).

So I think if backend is specified it will mean that only incremental fields will be synced and also this information should be passed to the destination as otherwise it can't know if we are syncing incremental source or not.

hermanschaaf · 2023-01-12T08:47:24Z

If we specify incremental, only incremental tables should be synced as otherwise it can cause issues with destinations such as gcs.

@yevgenypats what kind of issues are you thinking about here? Maybe let's sync up on this if you have time later :)

disq · 2023-01-12T10:13:55Z

@yevgenypats backend could've been nil before (not specifying the option would make it nil) so it worked for non-incrementals. I think???

disq · 2023-01-12T10:15:45Z

I don't think incremental-only should be automatically decided based on backend. If the user wants to sync incrementals on short periods but don't spam the full resources, they can have separate configs and use skip_tables. (Could also add an only_incremental bool option for convenience)

hermanschaaf

Maybe let's catch up about the new changes? I think we should consider some alternatives

hermanschaaf · 2023-01-12T13:50:06Z

specs/source.go

+	// Enable incremental syncing for tables that supports that
+	Incremental Incremental `json:"incremental,omitempty"`
+	// Sync on
+	OnlyIncrementalTables bool `json:"only_incremental_tables,omitempty"`


What is the use case for the OnlyIncrementalTables boolean? I don't understand why the same couldn't be achieved by splitting your config into incremental / non-incremental tables.

It would be hard to determine which tables are incremental and generate a config and keep it up to date. This way you can have one config with all tables, fetched daily, and another one, fetched hourly, updating only incrementals (taking shorter bursts, not wasting API limits and other resources)

True, but I think keeping your list of tables up-to-date (which to include, which to skip) and how regularly you want to sync each is a problem separate from incremental-ness, adding this one boolean is not going to solve it except maybe for a narrow use case

hermanschaaf · 2023-01-12T13:52:44Z

specs/incremental.go

+func (r *Incremental) UnmarshalJSON(data []byte) (err error) {
+	var registry string
+	if err := json.Unmarshal(data, &registry); err != nil {
+		return err
+	}
+	if *r, err = IncrementalFromString(registry); err != nil {
+		return err
+	}
+	return nil
+}


Suggested change

func (r *Incremental) UnmarshalJSON(data []byte) (err error) {

var registry string

if err := json.Unmarshal(data, &registry); err != nil {

return err

}

if *r, err = IncrementalFromString(registry); err != nil {

return err

}

return nil

}

func (r *Incremental) UnmarshalJSON(data []byte) (err error) {

var incremental string

if err := json.Unmarshal(data, &incremental); err != nil {

return err

}

if *r, err = IncrementalFromString(incremental); err != nil {

return err

}

return nil

}

hermanschaaf · 2023-01-12T13:52:59Z

specs/incremental.go

+	case "both":
+		return IncrementalBoth, nil
+	default:
+		return IncrementalNone, fmt.Errorf("unknown registry %s", s)


Suggested change

return IncrementalNone, fmt.Errorf("unknown registry %s", s)

return IncrementalNone, fmt.Errorf("unknown incremental value %s", s)

hermanschaaf · 2023-01-12T13:53:52Z

specs/incremental.go

+	IncrementalNone Incremental = iota
+	IncrementalTablesOnly
+	IncrementalBoth


What is the behavior expected to be for all these settings? What is the difference between setting IncrementalTablesOnly here and using the OnlyIncrementalTables boolean at the source spec level?

yevgenypats · 2023-01-12T15:52:35Z

Removed Incremental per discussion and using only backend config.

hermanschaaf · 2023-01-12T15:55:57Z

internal/servers/destinations.go

+	sourceSpec = specs.Source{
+		Name: sourceName,
+	}


Why are we overwriting sourceSpec, shortly after unmarshaling from r.SourceSpec?

good catch. a copy paste from the previous if

hermanschaaf

Looks good, just had the one question about overwriting the variable shortly after unmarshalling it

🤖 I have created a release *beep* *boop* --- ## [1.25.1](v1.25.0...v1.25.1) (2023-01-14) ### Bug Fixes * Change options for new client ([#603](#603)) ([f548a54](f548a54)) * PK Addition Order ([#607](#607)) ([eff40e7](eff40e7)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).

fix: Change new API

8b41f5b

yevgenypats requested a review from hermanschaaf January 11, 2023 22:25

github-actions bot added the fix label Jan 11, 2023

fmt

cf7c6b4

fix more

6862935

github-actions bot added fix and removed fix labels Jan 11, 2023

fix more

67e3611

fix tests

6c14168

hermanschaaf approved these changes Jan 12, 2023

View reviewed changes

fixes

53cd0d2

yevgenypats requested a review from hermanschaaf January 12, 2023 13:33

hermanschaaf reviewed Jan 12, 2023

View reviewed changes

yevgenypats added 4 commits January 12, 2023 17:32

remove incremental

1d09226

fmt

beb836f

keep backward compat

41f071e

lint fix

ec18430

yevgenypats requested a review from hermanschaaf January 12, 2023 15:52

Merge branch 'main' into fix/new_api

39000b8

hermanschaaf reviewed Jan 12, 2023

View reviewed changes

hermanschaaf approved these changes Jan 12, 2023

View reviewed changes

fix review

5c20c1c

yevgenypats added the automerge label Jan 12, 2023

kodiakhq bot merged commit f548a54 into main Jan 12, 2023

kodiakhq bot deleted the fix/new_api branch January 12, 2023 16:13

cq-bot mentioned this pull request Jan 12, 2023

chore(main): Release v1.25.1 #605

Merged

	return IncrementalNone, fmt.Errorf("unknown registry %s", s)
	return IncrementalNone, fmt.Errorf("unknown incremental value %s", s)

fix: Change options for new client #603

fix: Change options for new client #603

Uh oh!

Conversation

yevgenypats commented Jan 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Jan 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

⏱️ Benchmark results

Uh oh!

yevgenypats commented Jan 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hermanschaaf commented Jan 12, 2023

Uh oh!

disq commented Jan 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

disq commented Jan 12, 2023

Uh oh!

hermanschaaf left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yevgenypats commented Jan 12, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hermanschaaf left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

yevgenypats commented Jan 11, 2023 •

edited

Loading

github-actions bot commented Jan 11, 2023 •

edited

Loading

yevgenypats commented Jan 11, 2023 •

edited

Loading

disq commented Jan 12, 2023 •

edited

Loading