You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by "TSultanov (via GitHub)" <gi...@apache.org> on 2023/03/29 14:01:15 UTC
[GitHub] [beam] TSultanov opened a new pull request, #26026: [Do not merge] [Playground] Move functions to modify saved snippets to cloudfunctions
TSultanov opened a new pull request, #26026:
URL: https://github.com/apache/beam/pull/26026
**Please** add a meaningful description for your change here
------------------------
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
- [ ] Mention the appropriate issue in your description (for example: `addresses #123`), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, comment `fixes #<ISSUE NUMBER>` instead.
- [ ] Update `CHANGES.md` with noteworthy changes.
- [ ] If this contribution is large, please file an Apache [Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
See the [Contributor Guide](https://beam.apache.org/contribute) for more tips on [how to make review process smoother](https://beam.apache.org/contribute/get-started-contributing/#make-the-reviewers-job-easier).
To check the build health, please visit [https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md](https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md)
GitHub Actions Tests Status (on master branch)
------------------------------------------------------------------------------------------------
[](https://github.com/apache/beam/actions?query=workflow%3A%22Build+python+source+distribution+and+wheels%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Python+Tests%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Java+Tests%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Go+tests%22+branch%3Amaster+event%3Aschedule)
See [CI.md](https://github.com/apache/beam/blob/master/CI.md) for more information about GitHub Actions CI.
--
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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] TSultanov commented on a diff in pull request #26026: [Playground] Move modify saved snippets functionality to to cloudfunctions
Posted by "TSultanov (via GitHub)" <gi...@apache.org>.
TSultanov commented on code in PR #26026:
URL: https://github.com/apache/beam/pull/26026#discussion_r1183803809
##########
playground/backend/internal/db/datastore/datastore_db.go:
##########
@@ -63,19 +69,42 @@ func (d *Datastore) deleteObsoleteSnippets(ctx context.Context, snipKey *datasto
Namespace(utils.GetNamespace(ctx)).
FilterField("persistenceKey", "=", persistenceKey)
- // At the moment, datastore emulator doesn't allow != filters,
- // hence this crutches
- // https://cloud.google.com/datastore/docs/tools/datastore-emulator#known_issues
- // When it's fixed, post-query filter could be replaced with
- //
- // FilterField("__key__", "!=", snipKey)
+ // At the moment, datastore emulator doesn't allow != filters,
+ // hence this crutches
+ // https://cloud.google.com/datastore/docs/tools/datastore-emulator#known_issues
+ // When it's fixed, post-query filter could be replaced with
+ //
+ // FilterField("__key__", "!=", snipKey)
return d.deleteSnippets(ctx, snippetQuery, snipKey)
}
-// PutSnippet puts the snippet entity to datastore
+// PutSnippet puts the snippet entity to datastore using cloud function proxy
func (d *Datastore) PutSnippet(ctx context.Context, snipId string, snip *entity.Snippet) error {
logger.Debugf("putting snippet %q, persistent key %q...", snipId, snip.Snippet.PersistenceKey)
+
+ var err error
+ if d.externalFunctions != nil {
+ err = d.externalFunctions.PutSnippet(ctx, snipId, snip)
+ }
+ if err != nil || d.externalFunctions == nil {
+ if err != nil {
+ logger.Errorf("Datastore: PutSnippet(): error during the PutSnippet() call to the cloud function, "+
+ "accessing the datastore directly, err: %s\n", err.Error())
+ }
+ if d.externalFunctions == nil {
+ logger.Warnf("Datastore: PutSnippet(): external functions are not set, " +
+ "accessing the datastore directly")
+ }
+ return d.PutSnippetDirect(ctx, snipId, snip)
+ }
+
+ return nil
+}
+
+// PutSnippetDirect puts the snippet entity to datastore
+func (d *Datastore) PutSnippetDirect(ctx context.Context, snipId string, snip *entity.Snippet) error {
Review Comment:
It's been called from [here](https://github.com/akvelon/beam/blob/7f3c273183f0f12010bac2268047b014efcc708a/playground/backend/functions.go#L97)
--
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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] TSultanov commented on a diff in pull request #26026: [Playground] Move modify saved snippets functionality to to cloudfunctions
Posted by "TSultanov (via GitHub)" <gi...@apache.org>.
TSultanov commented on code in PR #26026:
URL: https://github.com/apache/beam/pull/26026#discussion_r1183801086
##########
playground/backend/internal/db/datastore/datastore_db.go:
##########
@@ -63,19 +69,42 @@ func (d *Datastore) deleteObsoleteSnippets(ctx context.Context, snipKey *datasto
Namespace(utils.GetNamespace(ctx)).
FilterField("persistenceKey", "=", persistenceKey)
- // At the moment, datastore emulator doesn't allow != filters,
- // hence this crutches
- // https://cloud.google.com/datastore/docs/tools/datastore-emulator#known_issues
- // When it's fixed, post-query filter could be replaced with
- //
- // FilterField("__key__", "!=", snipKey)
+ // At the moment, datastore emulator doesn't allow != filters,
+ // hence this crutches
+ // https://cloud.google.com/datastore/docs/tools/datastore-emulator#known_issues
+ // When it's fixed, post-query filter could be replaced with
+ //
+ // FilterField("__key__", "!=", snipKey)
return d.deleteSnippets(ctx, snippetQuery, snipKey)
}
-// PutSnippet puts the snippet entity to datastore
+// PutSnippet puts the snippet entity to datastore using cloud function proxy
func (d *Datastore) PutSnippet(ctx context.Context, snipId string, snip *entity.Snippet) error {
logger.Debugf("putting snippet %q, persistent key %q...", snipId, snip.Snippet.PersistenceKey)
+
+ var err error
+ if d.externalFunctions != nil {
+ err = d.externalFunctions.PutSnippet(ctx, snipId, snip)
+ }
+ if err != nil || d.externalFunctions == nil {
+ if err != nil {
+ logger.Errorf("Datastore: PutSnippet(): error during the PutSnippet() call to the cloud function, "+
+ "accessing the datastore directly, err: %s\n", err.Error())
+ }
+ if d.externalFunctions == nil {
+ logger.Warnf("Datastore: PutSnippet(): external functions are not set, " +
+ "accessing the datastore directly")
+ }
+ return d.PutSnippetDirect(ctx, snipId, snip)
Review Comment:
The fallback is useful for local development as with it the application will work with datastore emulator regardless of whether functions runtime was also started or not.
In the GKE deployment the fallback will fail as there are no permissions to write to the datastore.
Removing the fallback will also complicate ToB integration tests, though having them use the emulated cloud function will probably make them more true to life
--
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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] rshamunov commented on a diff in pull request #26026: [Playground] Move functions to modify saved snippets to cloudfunctions
Posted by "rshamunov (via GitHub)" <gi...@apache.org>.
rshamunov commented on code in PR #26026:
URL: https://github.com/apache/beam/pull/26026#discussion_r1161740936
##########
playground/infrastructure/helm-playground/templates/deployment-router.yml:
##########
@@ -42,6 +42,12 @@ spec:
value: {{ .Values.redis_ip }}
- name: NUM_PARALLEL_JOBS
value: "5"
+ - name: CLEANUP_SNIPPETS_FUNCTIONS_URL
Review Comment:
Where is DATASTORE_NAMESPACE?
--
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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] github-actions[bot] commented on pull request #26026: [Playground] Move modify saved snippets functionality to to cloudfunctions
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #26026:
URL: https://github.com/apache/beam/pull/26026#issuecomment-1531737903
Assigning reviewers. If you would like to opt out of this review, comment `assign to next reviewer`:
R: @damccorm for label build.
Available commands:
- `stop reviewer notifications` - opt out of the automated review tooling
- `remind me after tests pass` - tag the comment author after tests pass
- `waiting on author` - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)
The PR bot will only process comments in the main thread (not review comments).
--
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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] TSultanov commented on a diff in pull request #26026: [Playground] Move modify saved snippets functionality to to cloudfunctions
Posted by "TSultanov (via GitHub)" <gi...@apache.org>.
TSultanov commented on code in PR #26026:
URL: https://github.com/apache/beam/pull/26026#discussion_r1183801086
##########
playground/backend/internal/db/datastore/datastore_db.go:
##########
@@ -63,19 +69,42 @@ func (d *Datastore) deleteObsoleteSnippets(ctx context.Context, snipKey *datasto
Namespace(utils.GetNamespace(ctx)).
FilterField("persistenceKey", "=", persistenceKey)
- // At the moment, datastore emulator doesn't allow != filters,
- // hence this crutches
- // https://cloud.google.com/datastore/docs/tools/datastore-emulator#known_issues
- // When it's fixed, post-query filter could be replaced with
- //
- // FilterField("__key__", "!=", snipKey)
+ // At the moment, datastore emulator doesn't allow != filters,
+ // hence this crutches
+ // https://cloud.google.com/datastore/docs/tools/datastore-emulator#known_issues
+ // When it's fixed, post-query filter could be replaced with
+ //
+ // FilterField("__key__", "!=", snipKey)
return d.deleteSnippets(ctx, snippetQuery, snipKey)
}
-// PutSnippet puts the snippet entity to datastore
+// PutSnippet puts the snippet entity to datastore using cloud function proxy
func (d *Datastore) PutSnippet(ctx context.Context, snipId string, snip *entity.Snippet) error {
logger.Debugf("putting snippet %q, persistent key %q...", snipId, snip.Snippet.PersistenceKey)
+
+ var err error
+ if d.externalFunctions != nil {
+ err = d.externalFunctions.PutSnippet(ctx, snipId, snip)
+ }
+ if err != nil || d.externalFunctions == nil {
+ if err != nil {
+ logger.Errorf("Datastore: PutSnippet(): error during the PutSnippet() call to the cloud function, "+
+ "accessing the datastore directly, err: %s\n", err.Error())
+ }
+ if d.externalFunctions == nil {
+ logger.Warnf("Datastore: PutSnippet(): external functions are not set, " +
+ "accessing the datastore directly")
+ }
+ return d.PutSnippetDirect(ctx, snipId, snip)
Review Comment:
The fallback is useful for local development as with it the application will work with datastore emulator regardless of whether functions runtime was also started or not.
In the GKE deployment the fallback will fail as there are no permissions to write to the datastore
--
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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] TSultanov commented on a diff in pull request #26026: [Playground] Move modify saved snippets functionality to to cloudfunctions
Posted by "TSultanov (via GitHub)" <gi...@apache.org>.
TSultanov commented on code in PR #26026:
URL: https://github.com/apache/beam/pull/26026#discussion_r1183801086
##########
playground/backend/internal/db/datastore/datastore_db.go:
##########
@@ -63,19 +69,42 @@ func (d *Datastore) deleteObsoleteSnippets(ctx context.Context, snipKey *datasto
Namespace(utils.GetNamespace(ctx)).
FilterField("persistenceKey", "=", persistenceKey)
- // At the moment, datastore emulator doesn't allow != filters,
- // hence this crutches
- // https://cloud.google.com/datastore/docs/tools/datastore-emulator#known_issues
- // When it's fixed, post-query filter could be replaced with
- //
- // FilterField("__key__", "!=", snipKey)
+ // At the moment, datastore emulator doesn't allow != filters,
+ // hence this crutches
+ // https://cloud.google.com/datastore/docs/tools/datastore-emulator#known_issues
+ // When it's fixed, post-query filter could be replaced with
+ //
+ // FilterField("__key__", "!=", snipKey)
return d.deleteSnippets(ctx, snippetQuery, snipKey)
}
-// PutSnippet puts the snippet entity to datastore
+// PutSnippet puts the snippet entity to datastore using cloud function proxy
func (d *Datastore) PutSnippet(ctx context.Context, snipId string, snip *entity.Snippet) error {
logger.Debugf("putting snippet %q, persistent key %q...", snipId, snip.Snippet.PersistenceKey)
+
+ var err error
+ if d.externalFunctions != nil {
+ err = d.externalFunctions.PutSnippet(ctx, snipId, snip)
+ }
+ if err != nil || d.externalFunctions == nil {
+ if err != nil {
+ logger.Errorf("Datastore: PutSnippet(): error during the PutSnippet() call to the cloud function, "+
+ "accessing the datastore directly, err: %s\n", err.Error())
+ }
+ if d.externalFunctions == nil {
+ logger.Warnf("Datastore: PutSnippet(): external functions are not set, " +
+ "accessing the datastore directly")
+ }
+ return d.PutSnippetDirect(ctx, snipId, snip)
Review Comment:
The fallback is useful for local development as with it the application will work with datastore emulator regardless of whether functions runtime was also started or not.
In the GKE deployment the fallback will fail as there are no permissions to write to the datastore.
Removing the fallback will also complicate ToB integration tests
--
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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] codecov[bot] commented on pull request #26026: [Playground] Move functions to modify saved snippets to cloudfunctions
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #26026:
URL: https://github.com/apache/beam/pull/26026#issuecomment-1500210563
## [Codecov](https://codecov.io/gh/apache/beam/pull/26026?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#26026](https://codecov.io/gh/apache/beam/pull/26026?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (75489cd) into [master](https://codecov.io/gh/apache/beam/commit/c90f5b9dd9af5e926685fb8574c545b4108d831d?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (c90f5b9) will **decrease** coverage by `0.02%`.
> The diff coverage is `n/a`.
```diff
@@ Coverage Diff @@
## master #26026 +/- ##
==========================================
- Coverage 71.36% 71.35% -0.02%
==========================================
Files 783 783
Lines 102937 102961 +24
==========================================
+ Hits 73464 73470 +6
- Misses 27997 28015 +18
Partials 1476 1476
```
| Flag | Coverage Δ | |
|---|---|---|
| python | `79.84% <ø> (-0.02%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
[see 5 files with indirect coverage changes](https://codecov.io/gh/apache/beam/pull/26026/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] TSultanov commented on a diff in pull request #26026: [Playground] Move modify saved snippets functionality to to cloudfunctions
Posted by "TSultanov (via GitHub)" <gi...@apache.org>.
TSultanov commented on code in PR #26026:
URL: https://github.com/apache/beam/pull/26026#discussion_r1183823636
##########
playground/backend/internal/tasks/task.go:
##########
@@ -34,11 +34,11 @@ func New(ctx context.Context) *ScheduledTask {
return &ScheduledTask{ctx: ctx, taskScheduler: chrono.NewDefaultTaskScheduler()}
}
-func (st *ScheduledTask) StartRemovingExtraSnippets(cron string, dayDiff int32, db db.Database) error {
+func (st *ScheduledTask) StartRemovingExtraSnippets(cron string, externalFunction external_functions.ExternalFunctions) error {
task, err := st.taskScheduler.ScheduleWithCron(func(ctx context.Context) {
logger.Info("ScheduledTask: StartRemovingExtraSnippets() is running...\n")
startDate := time.Now()
- if err := db.DeleteUnusedSnippets(ctx, dayDiff); err != nil {
+ if err := externalFunction.CleanupSnippets(ctx); err != nil {
Review Comment:
No particular reason for a direct call here, it's just there is not need for this function to continue to work without cloudfunctions as it's not involved in the integration testing of ToB
--
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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] TSultanov commented on a diff in pull request #26026: [Playground] Move modify saved snippets functionality to to cloudfunctions
Posted by "TSultanov (via GitHub)" <gi...@apache.org>.
TSultanov commented on code in PR #26026:
URL: https://github.com/apache/beam/pull/26026#discussion_r1183801086
##########
playground/backend/internal/db/datastore/datastore_db.go:
##########
@@ -63,19 +69,42 @@ func (d *Datastore) deleteObsoleteSnippets(ctx context.Context, snipKey *datasto
Namespace(utils.GetNamespace(ctx)).
FilterField("persistenceKey", "=", persistenceKey)
- // At the moment, datastore emulator doesn't allow != filters,
- // hence this crutches
- // https://cloud.google.com/datastore/docs/tools/datastore-emulator#known_issues
- // When it's fixed, post-query filter could be replaced with
- //
- // FilterField("__key__", "!=", snipKey)
+ // At the moment, datastore emulator doesn't allow != filters,
+ // hence this crutches
+ // https://cloud.google.com/datastore/docs/tools/datastore-emulator#known_issues
+ // When it's fixed, post-query filter could be replaced with
+ //
+ // FilterField("__key__", "!=", snipKey)
return d.deleteSnippets(ctx, snippetQuery, snipKey)
}
-// PutSnippet puts the snippet entity to datastore
+// PutSnippet puts the snippet entity to datastore using cloud function proxy
func (d *Datastore) PutSnippet(ctx context.Context, snipId string, snip *entity.Snippet) error {
logger.Debugf("putting snippet %q, persistent key %q...", snipId, snip.Snippet.PersistenceKey)
+
+ var err error
+ if d.externalFunctions != nil {
+ err = d.externalFunctions.PutSnippet(ctx, snipId, snip)
+ }
+ if err != nil || d.externalFunctions == nil {
+ if err != nil {
+ logger.Errorf("Datastore: PutSnippet(): error during the PutSnippet() call to the cloud function, "+
+ "accessing the datastore directly, err: %s\n", err.Error())
+ }
+ if d.externalFunctions == nil {
+ logger.Warnf("Datastore: PutSnippet(): external functions are not set, " +
+ "accessing the datastore directly")
+ }
+ return d.PutSnippetDirect(ctx, snipId, snip)
Review Comment:
The fallback is useful for local development as with it the application will work with datastore emulator regardless of whether functions runtime was also started or not.
In the GKE deployment the fallback will fail as there are no permissions to write to the datastore.
Removing the fallback will also complicate ToB integration tests, though having them use the emulated cloud function will probably make them more true to life. But I'd like to avoid complicating the local development deployment procedures as much as possible
--
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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] TSultanov commented on a diff in pull request #26026: [Playground] Move modify saved snippets functionality to to cloudfunctions
Posted by "TSultanov (via GitHub)" <gi...@apache.org>.
TSultanov commented on code in PR #26026:
URL: https://github.com/apache/beam/pull/26026#discussion_r1183823636
##########
playground/backend/internal/tasks/task.go:
##########
@@ -34,11 +34,11 @@ func New(ctx context.Context) *ScheduledTask {
return &ScheduledTask{ctx: ctx, taskScheduler: chrono.NewDefaultTaskScheduler()}
}
-func (st *ScheduledTask) StartRemovingExtraSnippets(cron string, dayDiff int32, db db.Database) error {
+func (st *ScheduledTask) StartRemovingExtraSnippets(cron string, externalFunction external_functions.ExternalFunctions) error {
task, err := st.taskScheduler.ScheduleWithCron(func(ctx context.Context) {
logger.Info("ScheduledTask: StartRemovingExtraSnippets() is running...\n")
startDate := time.Now()
- if err := db.DeleteUnusedSnippets(ctx, dayDiff); err != nil {
+ if err := externalFunction.CleanupSnippets(ctx); err != nil {
Review Comment:
There is not need for this function to continue to work without cloudfunctions, as it's not involved in the integration testing of ToB. Because of this there is not reason for making a fallback logic and a direct call is enough
--
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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] damccorm commented on a diff in pull request #26026: [Playground] Move modify saved snippets functionality to to cloudfunctions
Posted by "damccorm (via GitHub)" <gi...@apache.org>.
damccorm commented on code in PR #26026:
URL: https://github.com/apache/beam/pull/26026#discussion_r1183743346
##########
playground/backend/internal/db/datastore/datastore_db.go:
##########
@@ -63,19 +69,42 @@ func (d *Datastore) deleteObsoleteSnippets(ctx context.Context, snipKey *datasto
Namespace(utils.GetNamespace(ctx)).
FilterField("persistenceKey", "=", persistenceKey)
- // At the moment, datastore emulator doesn't allow != filters,
- // hence this crutches
- // https://cloud.google.com/datastore/docs/tools/datastore-emulator#known_issues
- // When it's fixed, post-query filter could be replaced with
- //
- // FilterField("__key__", "!=", snipKey)
+ // At the moment, datastore emulator doesn't allow != filters,
+ // hence this crutches
+ // https://cloud.google.com/datastore/docs/tools/datastore-emulator#known_issues
+ // When it's fixed, post-query filter could be replaced with
+ //
+ // FilterField("__key__", "!=", snipKey)
return d.deleteSnippets(ctx, snippetQuery, snipKey)
}
-// PutSnippet puts the snippet entity to datastore
+// PutSnippet puts the snippet entity to datastore using cloud function proxy
func (d *Datastore) PutSnippet(ctx context.Context, snipId string, snip *entity.Snippet) error {
logger.Debugf("putting snippet %q, persistent key %q...", snipId, snip.Snippet.PersistenceKey)
+
+ var err error
+ if d.externalFunctions != nil {
+ err = d.externalFunctions.PutSnippet(ctx, snipId, snip)
+ }
+ if err != nil || d.externalFunctions == nil {
+ if err != nil {
+ logger.Errorf("Datastore: PutSnippet(): error during the PutSnippet() call to the cloud function, "+
+ "accessing the datastore directly, err: %s\n", err.Error())
+ }
+ if d.externalFunctions == nil {
+ logger.Warnf("Datastore: PutSnippet(): external functions are not set, " +
+ "accessing the datastore directly")
+ }
+ return d.PutSnippetDirect(ctx, snipId, snip)
+ }
+
+ return nil
+}
+
+// PutSnippetDirect puts the snippet entity to datastore
+func (d *Datastore) PutSnippetDirect(ctx context.Context, snipId string, snip *entity.Snippet) error {
Review Comment:
Should this be private (`putSnippetDirect`)?
##########
playground/backend/internal/tasks/task.go:
##########
@@ -34,11 +34,11 @@ func New(ctx context.Context) *ScheduledTask {
return &ScheduledTask{ctx: ctx, taskScheduler: chrono.NewDefaultTaskScheduler()}
}
-func (st *ScheduledTask) StartRemovingExtraSnippets(cron string, dayDiff int32, db db.Database) error {
+func (st *ScheduledTask) StartRemovingExtraSnippets(cron string, externalFunction external_functions.ExternalFunctions) error {
task, err := st.taskScheduler.ScheduleWithCron(func(ctx context.Context) {
logger.Info("ScheduledTask: StartRemovingExtraSnippets() is running...\n")
startDate := time.Now()
- if err := db.DeleteUnusedSnippets(ctx, dayDiff); err != nil {
+ if err := externalFunction.CleanupSnippets(ctx); err != nil {
Review Comment:
Any reason to call the externalFunction directly here, but use datastore_db as the mechanism for doing it elsewhere?
##########
playground/backend/internal/db/datastore/datastore_db.go:
##########
@@ -63,19 +69,42 @@ func (d *Datastore) deleteObsoleteSnippets(ctx context.Context, snipKey *datasto
Namespace(utils.GetNamespace(ctx)).
FilterField("persistenceKey", "=", persistenceKey)
- // At the moment, datastore emulator doesn't allow != filters,
- // hence this crutches
- // https://cloud.google.com/datastore/docs/tools/datastore-emulator#known_issues
- // When it's fixed, post-query filter could be replaced with
- //
- // FilterField("__key__", "!=", snipKey)
+ // At the moment, datastore emulator doesn't allow != filters,
+ // hence this crutches
+ // https://cloud.google.com/datastore/docs/tools/datastore-emulator#known_issues
+ // When it's fixed, post-query filter could be replaced with
+ //
+ // FilterField("__key__", "!=", snipKey)
return d.deleteSnippets(ctx, snippetQuery, snipKey)
}
-// PutSnippet puts the snippet entity to datastore
+// PutSnippet puts the snippet entity to datastore using cloud function proxy
func (d *Datastore) PutSnippet(ctx context.Context, snipId string, snip *entity.Snippet) error {
logger.Debugf("putting snippet %q, persistent key %q...", snipId, snip.Snippet.PersistenceKey)
+
+ var err error
+ if d.externalFunctions != nil {
+ err = d.externalFunctions.PutSnippet(ctx, snipId, snip)
+ }
+ if err != nil || d.externalFunctions == nil {
+ if err != nil {
+ logger.Errorf("Datastore: PutSnippet(): error during the PutSnippet() call to the cloud function, "+
+ "accessing the datastore directly, err: %s\n", err.Error())
+ }
+ if d.externalFunctions == nil {
+ logger.Warnf("Datastore: PutSnippet(): external functions are not set, " +
+ "accessing the datastore directly")
+ }
+ return d.PutSnippetDirect(ctx, snipId, snip)
Review Comment:
Why do we need this fallback? It seems concerning/like it opens us up to the same issues this PR is trying to avoid. Are there ever cases where the fallback is expected/valid?
##########
playground/backend/internal/db/datastore/datastore_db.go:
##########
@@ -63,19 +69,42 @@ func (d *Datastore) deleteObsoleteSnippets(ctx context.Context, snipKey *datasto
Namespace(utils.GetNamespace(ctx)).
FilterField("persistenceKey", "=", persistenceKey)
- // At the moment, datastore emulator doesn't allow != filters,
- // hence this crutches
- // https://cloud.google.com/datastore/docs/tools/datastore-emulator#known_issues
- // When it's fixed, post-query filter could be replaced with
- //
- // FilterField("__key__", "!=", snipKey)
+ // At the moment, datastore emulator doesn't allow != filters,
+ // hence this crutches
+ // https://cloud.google.com/datastore/docs/tools/datastore-emulator#known_issues
+ // When it's fixed, post-query filter could be replaced with
+ //
+ // FilterField("__key__", "!=", snipKey)
return d.deleteSnippets(ctx, snippetQuery, snipKey)
}
-// PutSnippet puts the snippet entity to datastore
+// PutSnippet puts the snippet entity to datastore using cloud function proxy
func (d *Datastore) PutSnippet(ctx context.Context, snipId string, snip *entity.Snippet) error {
logger.Debugf("putting snippet %q, persistent key %q...", snipId, snip.Snippet.PersistenceKey)
+
+ var err error
+ if d.externalFunctions != nil {
+ err = d.externalFunctions.PutSnippet(ctx, snipId, snip)
+ }
+ if err != nil || d.externalFunctions == nil {
+ if err != nil {
+ logger.Errorf("Datastore: PutSnippet(): error during the PutSnippet() call to the cloud function, "+
+ "accessing the datastore directly, err: %s\n", err.Error())
+ }
+ if d.externalFunctions == nil {
+ logger.Warnf("Datastore: PutSnippet(): external functions are not set, " +
+ "accessing the datastore directly")
+ }
+ return d.PutSnippetDirect(ctx, snipId, snip)
Review Comment:
Same question applies below (both this and the private question)
##########
playground/backend/internal/tasks/task.go:
##########
@@ -34,11 +34,11 @@ func New(ctx context.Context) *ScheduledTask {
return &ScheduledTask{ctx: ctx, taskScheduler: chrono.NewDefaultTaskScheduler()}
}
-func (st *ScheduledTask) StartRemovingExtraSnippets(cron string, dayDiff int32, db db.Database) error {
+func (st *ScheduledTask) StartRemovingExtraSnippets(cron string, externalFunction external_functions.ExternalFunctions) error {
task, err := st.taskScheduler.ScheduleWithCron(func(ctx context.Context) {
logger.Info("ScheduledTask: StartRemovingExtraSnippets() is running...\n")
startDate := time.Now()
- if err := db.DeleteUnusedSnippets(ctx, dayDiff); err != nil {
+ if err := externalFunction.CleanupSnippets(ctx); err != nil {
Review Comment:
I think that sometimes using datastore_db and sometimes using the externalFunctions is a little confusing, I'd prefer we standardize on one (my vote would be to standardize on calling externalFunctions directly since they're not inherently tied to always invoking the datastore_db.
--
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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] damccorm merged pull request #26026: [Playground] Move modify saved snippets functionality to to cloudfunctions
Posted by "damccorm (via GitHub)" <gi...@apache.org>.
damccorm merged PR #26026:
URL: https://github.com/apache/beam/pull/26026
--
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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] damccorm commented on a diff in pull request #26026: [Playground] Move modify saved snippets functionality to to cloudfunctions
Posted by "damccorm (via GitHub)" <gi...@apache.org>.
damccorm commented on code in PR #26026:
URL: https://github.com/apache/beam/pull/26026#discussion_r1184123293
##########
playground/backend/internal/tasks/task.go:
##########
@@ -34,11 +34,11 @@ func New(ctx context.Context) *ScheduledTask {
return &ScheduledTask{ctx: ctx, taskScheduler: chrono.NewDefaultTaskScheduler()}
}
-func (st *ScheduledTask) StartRemovingExtraSnippets(cron string, dayDiff int32, db db.Database) error {
+func (st *ScheduledTask) StartRemovingExtraSnippets(cron string, externalFunction external_functions.ExternalFunctions) error {
task, err := st.taskScheduler.ScheduleWithCron(func(ctx context.Context) {
logger.Info("ScheduledTask: StartRemovingExtraSnippets() is running...\n")
startDate := time.Now()
- if err := db.DeleteUnusedSnippets(ctx, dayDiff); err != nil {
+ if err := externalFunction.CleanupSnippets(ctx); err != nil {
Review Comment:
I think I would like for this line to be made clearer in the future, but I'm ok moving forward with it as is for now.
--
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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] TSultanov commented on a diff in pull request #26026: [Playground] Move modify saved snippets functionality to to cloudfunctions
Posted by "TSultanov (via GitHub)" <gi...@apache.org>.
TSultanov commented on code in PR #26026:
URL: https://github.com/apache/beam/pull/26026#discussion_r1183803809
##########
playground/backend/internal/db/datastore/datastore_db.go:
##########
@@ -63,19 +69,42 @@ func (d *Datastore) deleteObsoleteSnippets(ctx context.Context, snipKey *datasto
Namespace(utils.GetNamespace(ctx)).
FilterField("persistenceKey", "=", persistenceKey)
- // At the moment, datastore emulator doesn't allow != filters,
- // hence this crutches
- // https://cloud.google.com/datastore/docs/tools/datastore-emulator#known_issues
- // When it's fixed, post-query filter could be replaced with
- //
- // FilterField("__key__", "!=", snipKey)
+ // At the moment, datastore emulator doesn't allow != filters,
+ // hence this crutches
+ // https://cloud.google.com/datastore/docs/tools/datastore-emulator#known_issues
+ // When it's fixed, post-query filter could be replaced with
+ //
+ // FilterField("__key__", "!=", snipKey)
return d.deleteSnippets(ctx, snippetQuery, snipKey)
}
-// PutSnippet puts the snippet entity to datastore
+// PutSnippet puts the snippet entity to datastore using cloud function proxy
func (d *Datastore) PutSnippet(ctx context.Context, snipId string, snip *entity.Snippet) error {
logger.Debugf("putting snippet %q, persistent key %q...", snipId, snip.Snippet.PersistenceKey)
+
+ var err error
+ if d.externalFunctions != nil {
+ err = d.externalFunctions.PutSnippet(ctx, snipId, snip)
+ }
+ if err != nil || d.externalFunctions == nil {
+ if err != nil {
+ logger.Errorf("Datastore: PutSnippet(): error during the PutSnippet() call to the cloud function, "+
+ "accessing the datastore directly, err: %s\n", err.Error())
+ }
+ if d.externalFunctions == nil {
+ logger.Warnf("Datastore: PutSnippet(): external functions are not set, " +
+ "accessing the datastore directly")
+ }
+ return d.PutSnippetDirect(ctx, snipId, snip)
+ }
+
+ return nil
+}
+
+// PutSnippetDirect puts the snippet entity to datastore
+func (d *Datastore) PutSnippetDirect(ctx context.Context, snipId string, snip *entity.Snippet) error {
Review Comment:
It's called from [here](https://github.com/akvelon/beam/blob/7f3c273183f0f12010bac2268047b014efcc708a/playground/backend/functions.go#L97)
--
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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org