[BEAM-13632][Playground] Save catalog data to the cache by KhaninArtur · Pull Request #16493 · apache/beam

KhaninArtur · 2022-01-12T14:43:59Z

BEAM-13632 The catalog is now saved into the cache at the server startup.

Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

Choose reviewer(s) and mention them in a comment (R: @username).
Format the pull request title like [BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replace BEAM-XXX with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
Update CHANGES.md with noteworthy changes.
If this contribution is large, please file an Apache Individual Contributor License Agreement.

See the Contributor Guide for more tips on how to make review process smoother.

`ValidatesRunner` compliance status (on master branch)

Lang	ULR	Twister2
Go	---	---
Java
Python	---	---
XLang		---

Examples testing status on various runners

Lang	ULR	Dataflow	Flink	Samza	Spark	Twister2
Go	---	---	---	---	---	---	---
Java	---		---	---	---	---	---
Python	---	---	---	---	---	---	---
XLang	---	---	---	---	---	---	---

Post-Commit SDK/Transform Integration Tests Status (on master branch)

Go	Java	Python

Pre-Commit Tests Status (on master branch)

---	Java	Python	Go	Website	Whitespace	Typescript
Non-portable
Portable	---			---	---	---

See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.

GitHub Actions Tests Status (on master branch)

See CI.md for more information about GitHub Actions CI.

KhaninArtur · 2022-01-12T14:50:02Z

R: @ilya-kozyrev

playground/backend/cmd/server/controller.go

playground/backend/cmd/server/server.go

playground/backend/internal/utils/precompiled_objects_utils.go

playground/backend/internal/utils/precompiled_objects_utils_test.go

AydarZaynutdinov

Overall LGTM. Last minor suggestion.

AydarZaynutdinov · 2022-01-13T18:49:55Z

playground/backend/cmd/server/controller.go

+		} else {
+			logger.Errorf("GetPrecompiledObjects(): cache error: %s", err.Error())
+		}


Suggested change

} else {

logger.Errorf("GetPrecompiledObjects(): cache error: %s", err.Error())

}

}

logger.Errorf("GetPrecompiledObjects(): cache error: %s", err.Error())

Good idea, thanks!

pavel-avilov

LGTM

ilya-kozyrev · 2022-01-18T14:08:46Z

playground/backend/cmd/server/controller.go

 func (controller *playgroundController) GetPrecompiledObjects(ctx context.Context, info *pb.GetPrecompiledObjectsRequest) (*pb.GetPrecompiledObjectsResponse, error) {
-	bucket := cloud_bucket.New()
-	sdkToCategories, err := bucket.GetPrecompiledObjects(ctx, info.Sdk, info.Category)
+	if info.Sdk == pb.Sdk_SDK_UNSPECIFIED && info.Category == "" {


why we need this if here?

Changed per our discussion

ilya-kozyrev · 2022-01-18T14:13:40Z

playground/backend/internal/cache/cache.go

 	LogsIndex SubKey = "LOGS_INDEX"
+
+	// ExamplesCatalog is catalog of examples available in Playground
+	ExamplesCatalog SubKey = "EXAMPLES_CATALOG"


Instead of make workarounds with subkey without pipeline id. Can we extend the Cache interface and implementation with SetCatalog, GetKatalog?

You are right, changed

ilya-kozyrev · 2022-01-18T14:17:13Z

playground/backend/internal/utils/precompiled_objects_utils.go

 }
+
+// GetPrecompiledObjectsCatalogFromCache returns the precompiled objects catalog from the cache
+func GetPrecompiledObjectsCatalogFromCache(ctx context.Context, cacheService cache.Cache) ([]*pb.Categories, error) {


can we make this name shorter?
GetPrecompiledObjectsFromCache

This one became redundant and GetPrecompiledObjectsCatalogFromStorage I changed to GetCatalogFromStorage

…artup

AydarZaynutdinov · 2022-01-24T15:28:35Z

playground/backend/internal/cache/cache.go

+	// SetCatalog adds the given catalog to cache by ExamplesCatalog key.
+	SetCatalog(ctx context.Context, catalog []*pb.Categories) error
+
+	// GetCatalog return catalog from cache by ExamplesCatalog key.
+	GetCatalog(ctx context.Context) ([]*pb.Categories, error)


Why can't we use already existing methods GetValue and SetValue? Is it because we use Get/Set for catalog instead of HGet/HSet?

Yes, we save it without the workaround with nil pipelined as It was before.

AydarZaynutdinov · 2022-01-24T15:39:34Z

playground/backend/cmd/server/controller.go

+// - If SDK and category are unspecified in the request, gets the whole catalog from the cache
+//	- If there is no catalog in the cache, gets the catalog from the Storage and saves it to the cache
+// - If SDK or category is specified in the request, gets the specific catalog from the Storage


It seems that the comment here doesn't match the method logic. The method gets a catalog from the cache in all cases, not only if SDK and category are unspecified

Thanks for noticing, changed!

AydarZaynutdinov

LGTM

daria-malkova · 2022-01-25T09:43:14Z

playground/backend/internal/utils/precompiled_objects_utils.go

 }
+
+// GetCatalogFromStorage returns the precompiled objects catalog from the cloud storage
+func GetCatalogFromStorage(ctx context.Context) ([]*pb.Categories, error) {


Is it possible to add tests for this method?

No, its logic is connected with retrieving the data from the bucket – testing this will be integration testing, which we don't do now.

# Conflicts: # playground/backend/internal/cache/cache.go

daria-malkova

LGTM

ilya-kozyrev

LGTM Thanks

KhaninArtur · 2022-01-26T11:38:53Z

R: @pabloem

pabloem · 2022-01-31T18:27:34Z

this looks fine to me. thanks!

AydarZaynutdinov suggested changes Jan 12, 2022

View reviewed changes

AydarZaynutdinov approved these changes Jan 13, 2022

View reviewed changes

pavel-avilov approved these changes Jan 18, 2022

View reviewed changes

ilya-kozyrev suggested changes Jan 18, 2022

View reviewed changes

pavel-avilov mentioned this pull request Jan 21, 2022

[BEAM-13633] [Playground] Implement method to get a default example for each SDKs #16484

Merged

4 tasks

Artur Khanin added 6 commits January 24, 2022 17:26

Added saving of precompiled objects catalog to cache at the server st…

43fc19b

…artup

Added caching of the catalog only in case of unspecified SDK

27b0157

Update regarding comments

b9b77b9

Update regarding comments

2187918

Simplified logging regarding comment

3655716

Updates regarding comments

7fe3d8b

KhaninArtur force-pushed the BEAM-13632-cache-examples-data branch from d93dc2b to 7fe3d8b Compare January 24, 2022 14:26

Update for environment_service_test.go

6830670

AydarZaynutdinov suggested changes Jan 24, 2022

View reviewed changes

Docstring update for GetPrecompiledObjects

c306231

AydarZaynutdinov approved these changes Jan 24, 2022

View reviewed changes

daria-malkova reviewed Jan 25, 2022

View reviewed changes

Merge branch 'master' into BEAM-13632-cache-examples-data

7cef6bf

# Conflicts: # playground/backend/internal/cache/cache.go

daria-malkova approved these changes Jan 26, 2022

View reviewed changes

ilya-kozyrev approved these changes Jan 26, 2022

View reviewed changes

pabloem merged commit 4e5db10 into apache:master Jan 31, 2022

Conversation

KhaninArtur commented Jan 12, 2022

ValidatesRunner compliance status (on master branch)

Examples testing status on various runners

Post-Commit SDK/Transform Integration Tests Status (on master branch)

Pre-Commit Tests Status (on master branch)

GitHub Actions Tests Status (on master branch)

Uh oh!

KhaninArtur commented Jan 12, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AydarZaynutdinov 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!

pavel-avilov 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!

ilya-kozyrev Jan 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AydarZaynutdinov 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!

daria-malkova left a comment

Choose a reason for hiding this comment

Uh oh!

ilya-kozyrev left a comment

Choose a reason for hiding this comment

Uh oh!

KhaninArtur commented Jan 26, 2022

Uh oh!

pabloem commented Jan 31, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

`ValidatesRunner` compliance status (on master branch)

ilya-kozyrev Jan 18, 2022 •

edited

Loading