Skip to content

[BEAM-12998] [Playground] Environment service - #15654

Merged
pabloem merged 1 commit into
apache:masterfrom
akvelon:BEAM-12998-Environment_service
Oct 25, 2021
Merged

[BEAM-12998] [Playground] Environment service#15654
pabloem merged 1 commit into
apache:masterfrom
akvelon:BEAM-12998-Environment_service

Conversation

@ilya-kozyrev

@ilya-kozyrevilya-kozyrev commented Oct 4, 2021•
edited
Loading

Copy link
Copy Markdown
Contributor
  1. Adding structures to manage environments that need for different parts of the app.
  • BeamEnvs contains all environments related to ApacheBeam. These will use to run pipelines
  • LogWriters contains writes for the logger. Writers should implement io.Writer interface. logging will be implemented in next PRs
  • ServerEnvs contains all environment variables that need to run the server.
  1. Adding service to manage there variables. Service operates with environment structures: ServerEnvs, LogWriters, BeamEnvs and contains methods to extract some values from os.env

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)

LangULRDataflowFlinkSamzaSparkTwister2
Go---Build StatusBuild StatusBuild StatusBuild Status---
JavaBuild StatusBuild Status
Build Status
Build Status
Build Status
Build Status
Build Status
Build Status
Build Status
Build Status		Build Status
Build Status		Build Status
Build Status
Build Status		Build Status
Python---Build Status
Build Status
Build Status		Build Status
Build Status		Build StatusBuild Status---
XLangBuild StatusBuild StatusBuild StatusBuild StatusBuild Status---

Examples testing status on various runners

LangULRDataflowFlinkSamzaSparkTwister2
Go---------------------
Java---Build Status
Build Status
Build Status
---------------
Python---------------------
XLang---------------------

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

GoJavaPython
Build StatusBuild StatusBuild Status
Build Status
Build Status

Pre-Commit Tests Status (on master branch)

---JavaPythonGoWebsiteWhitespaceTypescript
Non-portableBuild Status
Build Status
Build Status
Build Status
Build Status		Build StatusBuild StatusBuild StatusBuild Status
Portable---Build StatusBuild Status---------

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

GitHub Actions Tests Status (on master branch)

Build python source distribution and wheels
Python tests
Java tests

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

@ilya-kozyrev

ilya-kozyrev commented Oct 4, 2021

Copy link
Copy Markdown
ContributorAuthor

R: @pabloem@damondouglas

This was referenced Oct 4, 2021
Closed
Merged
damondouglas
damondouglas requested changes Oct 6, 2021
View reviewed changes
Comment threadplayground/backend/internal/environment/environment_service.go Outdated
Comment threadplayground/backend/internal/environment/environment_service.go Outdated
Comment threadplayground/backend/internal/environment/environment_service.go Outdated
Comment threadplayground/backend/internal/environment/environment_service_test.go Outdated
Comment threadplayground/backend/internal/environment/logs.go Outdated
import "io"

// LogWriters contains writes for logger. Writers should implement io.Writer interface.
type LogWriters struct {

@damondouglasdamondouglasOct 6, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May we consider having a separate internal/log package? I like that we are centralizing logging and I would additionally propose enforcing structured logging. I've seen two approaches. One, using the uber zap library. Another using the native go library but logging out a struct adherent to https://pkg.go.dev/google.golang.org/cloud/logging#Entry. In the second case, just the Entry is populated and written out to stdout or stderr without using logging.Client. In GCP, structured logs are automatically parsed with the applied key/value pairs. I've used both approaches and found the second more flexible as there are settings where one just wants a single line string in the log.

@ilya-kozyrevilya-kozyrevOct 18, 2021

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We've removed logs from this package and consider to implement them in a separate package in the new pr, will submit them soon :)

Comment threadplayground/backend/internal/environment/network.go Outdated
@ilya-kozyrev
ilya-kozyrevforce-pushed the BEAM-12998-Environment_service branch 2 times, most recently from cfb6ed4 to 465a5d5CompareOctober 6, 2021 23:37
@aaltay

aaltay commented Oct 15, 2021

Copy link
Copy Markdown
Member

What is the next step on this PR?

@ilya-kozyrev
ilya-kozyrevforce-pushed the BEAM-12998-Environment_service branch from e8f495d to 2a3f274CompareOctober 18, 2021 09:35
@pabloem

pabloem commented Oct 20, 2021

Copy link
Copy Markdown
Member

@damondouglas - it seems like @ilya-kozyrev addressed your comments. Is that correct?

@ilya-kozyrev
ilya-kozyrevforce-pushed the BEAM-12998-Environment_service branch from 2a3f274 to 0802804CompareOctober 20, 2021 17:39
@ilya-kozyrevilya-kozyrev mentioned this pull request Oct 21, 2021
Merged
4 tasks
damondouglas
damondouglas approved these changes Oct 25, 2021
View reviewed changes

@damondouglasdamondouglas left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pabloem LGTM

@ilya-kozyrevAydarZaynutdinovdaria.malkova
Add environment_service.go and structures for beam sdk, network envs,…
aa26b53
… cache and application envs
Co-authored-by: AydarZaynutdinov <aydar.zaynutdinov@akvelon.com>
Co-authored-by: daria.malkova <daria.malkova@akvelon.com>
@ilya-kozyrev
ilya-kozyrevforce-pushed the BEAM-12998-Environment_service branch from 980ffb3 to aa26b53CompareOctober 25, 2021 15:33
@ilya-kozyrev

ilya-kozyrev commented Oct 25, 2021

Copy link
Copy Markdown
ContributorAuthor

@pabloem rebased on master. Could you please merge it?

@pabloem
pabloem merged commit 835d19a into apache:masterOct 25, 2021
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@damondouglasdamondouglasdamondouglas approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@ilya-kozyrev@aaltay@pabloem@damondouglas