Adding Google Storage Requester pays feature to Golang SDK. #33236

LeoCBS · 2024-11-27T13:10:11Z

Setting UserProject on Google Storage Bucket operations to enable requester pays feature.

Requester pays project ID will come from environment variable named BILLING_PROJECT_ID

More information about Google storage requester pays feature here https://cloud.google.com/storage/docs/requester-pays

Setting UserProject on Google Storage Bucket operations to enable requester pays feature. Requester pays project ID will come from environment variable named `BILLING_PROJECT_ID` More information about Google storage requester pays feature here https://cloud.google.com/storage/docs/requester-pays

github-actions · 2024-11-27T13:39:31Z

Assigning reviewers. If you would like to opt out of this review, comment assign to next reviewer:

R: @lostluck for label go.
R: @chamikaramj for label io.

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).

github-actions · 2024-12-05T12:14:24Z

Reminder, please take a look at this pr: @lostluck @chamikaramj

github-actions · 2024-12-09T12:14:55Z

Assigning new set of reviewers because Pr has gone too long without review. If you would like to opt out of this review, comment assign to next reviewer:

R: @jrmccluskey for label go.
R: @ahmedabu98 for label io.

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)

codecov · 2024-12-11T00:30:46Z

Codecov Report

Attention: Patch coverage is 93.75000% with 1 line in your changes missing coverage. Please review.

Project coverage is 57.41%. Comparing base (ad8545c) to head (bfc9077).
Report is 1018 commits behind head on master.

Files with missing lines	Patch %	Lines
sdks/go/pkg/beam/io/filesystem/gcs/gcs.go	93.75%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #33236      +/-   ##
============================================
- Coverage     58.92%   57.41%   -1.51%     
+ Complexity     3112     1475    -1637     
============================================
  Files          1133      970     -163     
  Lines        174889   154431   -20458     
  Branches       3343     1076    -2267     
============================================
- Hits         103045    88661   -14384     
+ Misses        68495    63569    -4926     
+ Partials       3349     2201    -1148

Flag	Coverage Δ
go	`∅ <93.75%> (∅)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

lostluck

This largely looks good, and it does seem that the storage package code handles an empty string there properly, despite not being in the documentation.

https://pkg.go.dev/cloud.google.com/go/storage#BucketHandle.UserProject

The main concern I have is whether the environment variable is automatically set in GCE VMs or not.

Have you tried this on Google Cloud Dataflow? (it's OK if you haven't, or your usage of Beam doesn't require it, but it's something I'll need to verify to ensure the feature works well...).

Once you get back to me on that, I'm happy to merge this in.

lostluck · 2024-12-11T00:43:01Z

I think the only thing I don't really love is using an environment variables for this, as it's not necessarily going to be set or not, and there doesn't seem to be any standardization around it within Beam or GCP (outside of a few scattered unrelated examples).

It would probably be better to do something as a configurable hook at Pipeline submission time, that is triggered when a worker is initialized.

LeoCBS · 2024-12-12T11:21:29Z

Hi @lostluck, thank you for your feedback. I will change the implementation to get the billing project from the pipeline variable instead of using the environment variable.

lostluck · 2024-12-12T21:56:59Z

If it were me, I'd add a function to the GCS package that sets the value when the fs is constructed. eg. SetRequesterBillingProject(project string)

Then do something like the harness opts do for the side input cache:

Defines and installs the hook URN.

beam/sdks/go/pkg/beam/core/runtime/harness/cache_hooks.go

Line 50 in de5e8eb

hooks.RegisterHook("beam:go:hook:sideinputcache:capacity", hf)

Has a method to be called at before pipeline submission to enable the hook, and call SetRequesterBillingProject with the value stored in the hook. Note, it just needs to be an arbitrary string (which could then be any serialized data that you like). This gets stored as a pipeline option, so it can be retrieved worker side.

https://github.com/apache/beam/blob/master/sdks/go/pkg/beam/util/harnessopts/cache.go

This would affect the whole pipeline (just as this change does), but if a pipeline author wants to do trickier bits where the billing requester is changing, then we can figure out other options.
Ultimately, the goal is to enable most uses, but not all uses.

github-actions · 2024-12-20T12:14:18Z

Reminder, please take a look at this pr: @jrmccluskey @ahmedabu98

LeoCBS · 2024-12-22T22:27:10Z

@lostluck check if i understand your suggestion:

in file sdks/go/pkg/beam/io/filesystem/gcs/gcs.go i will register a hook to get billing project information:

var (
    billingProject string = ""
)

func init() {
	hf := func(opts []string) hooks.Hook {
		return hooks.Hook{
			Init: func(ctx context.Context) (context.Context, error) {
				if len(opts) == 0 {
					return ctx, nil
				}
				if len(opts) > 1 {
					return ctx, fmt.Errorf("expected 1 option, got %v: %v", len(opts), opts)
				}

				billingProject = opts[0]
				return ctx, nil
			},
		}
	}
	hooks.RegisterHook("beam:go:hook:gcstorage:requesterbillingproject", hf)
}

and on beam/sdks/go/pkg/beam/util/harnessopts/gcs.go i will create a function like this?

const (
	billingProjectHook = "beam:go:hook:requesterbillingproject:capacity"
)

func SetRequesterBillingProject(billingProject string) error {
	return hooks.EnableHook(billingProjectHook, billingProject)
}

I have some questions:

* who will call function SetRequesterBillingProject?
* How will i test if hook was called correcty?
* Will i passing billing project by CLI args? like the runner value, `--runner dataflow`?

github-actions · 2024-12-25T12:14:35Z

Assigning new set of reviewers because Pr has gone too long without review. If you would like to opt out of this review, comment assign to next reviewer:

R: @lostluck for label go.
R: @damondouglas for label io.

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)

lostluck · 2024-12-27T19:35:30Z

@LeoCBS Thanks for your patience over this holiday season.

That looks about right!
There's no need to put the enabling function in the harnessopts package. The harness options were split up for package privacy reasons. Due to how tightly bound this feature is to GCS, please keep enabling function in the GCS filesystem package.
Pipelines that would need to have that set will need to call it in the before pipeline submission in the binary's 'main'. It must be before pipeline submission, but doesn't have to be before beam.Init. Note that whatever is doing the call for this is allowed to use whatever environment variable you like. You can automate it that way if that's how you prefer. (eg. A final user specific package to call it at package init time, that looks for your environment variable, and then calls the enabler method.) (or as you suggest, a flag, that gets set by the binary, and then is used when calling the enabling function).
You can add a test in the GCS filesystem package, that calls hooks.RunInitHooks -> https://github.com/apache/beam/blob/master/sdks/go/pkg/beam/core/util/hooks/hooks.go#L115 That will execute the enabled hooks. You should be able to verify that the billingProject package variable was set as expected there.

lostluck

Just swapping this to requestChanges from approved since the change is being made anyway.

github-actions · 2025-01-07T12:14:21Z

Reminder, please take a look at this pr: @lostluck @damondouglas

github-actions · 2025-01-10T12:14:33Z

Assigning new set of reviewers because Pr has gone too long without review. If you would like to opt out of this review, comment assign to next reviewer:

R: @jrmccluskey for label go.
R: @chamikaramj for label io.

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)

github-actions · 2025-01-18T12:13:57Z

Reminder, please take a look at this pr: @jrmccluskey @chamikaramj

github-actions · 2025-01-22T12:14:23Z

Assigning new set of reviewers because Pr has gone too long without review. If you would like to opt out of this review, comment assign to next reviewer:

R: @lostluck for label go.
R: @damondouglas for label io.

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)

lostluck · 2025-02-13T22:01:31Z

waiting on author

LeoCBS · 2025-02-16T12:00:57Z

@lostluck sorry for the delay, I was on vacation. I'm getting back to work on this MR :)

github-actions · 2025-02-23T12:13:58Z

Reminder, please take a look at this pr: @lostluck @Abacn

github-actions · 2025-02-26T12:14:34Z

Assigning new set of reviewers because Pr has gone too long without review. If you would like to opt out of this review, comment assign to next reviewer:

R: @jrmccluskey for label go.
R: @chamikaramj for label io.

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)

lostluck · 2025-02-26T12:56:16Z

waiting on author

claudevdm · 2025-04-15T16:09:25Z

Hi @LeoCBS is this PR still being worked on?

LeoCBS · 2025-04-21T12:02:16Z

Hi @LeoCBS is this PR still being worked on?

Hi @claudevdm

I'm working on this issue, I should be finishing it soon.

github-actions · 2025-04-28T12:15:12Z

Reminder, please take a look at this pr: @jrmccluskey @chamikaramj

github-actions · 2025-05-01T12:14:37Z

Assigning new set of reviewers because Pr has gone too long without review. If you would like to opt out of this review, comment assign to next reviewer:

R: @lostluck for label go.
R: @chamikaramj for label io.

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)

LeoCBS · 2025-05-11T23:07:57Z

Hi @lostluck , sorry for the delay to update this PR, could you check if my changes makes sense? I'm just missing one last point, how can i assert that the value set by hook was correctly set?

LeoCBS · 2025-05-11T23:08:47Z

sdks/go/pkg/beam/io/filesystem/gcs/gcs_test.go

+	if err != nil {
+		t.Errorf("error to init hooks = %v", err)
+	}
+	//TODO how assert project configured by hook?


@lostluck how can i assert project set by hook?

Call: hooks.IsEnabled. It will return if the hook is enabled, and the options that were set.

https://pkg.go.dev/github.com/apache/beam/sdks/[email protected]/go/pkg/beam/core/util/hooks#IsEnabled

lostluck · 2025-05-11T23:43:25Z

sdks/go/pkg/beam/core/runtime/harness/file_system_hooks.go

+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package harness


Don't put this into the harness package. It can live in the GCS package, registered to the the hook. The harness already triggers hooks on worker startup.

harnessopts does things in the harness package because they're options for the harness, but that separation doesn't need to exist for the gcs package.

@lostluck could you review changes again?

lostluck · 2025-05-11T23:44:10Z

sdks/go/pkg/beam/io/filesystem/gcs/gcs_test.go

+	if err != nil {
+		t.Errorf("error to init hooks = %v", err)
+	}
+	//TODO how assert project configured by hook?


Call: hooks.IsEnabled. It will return if the hook is enabled, and the options that were set.

https://pkg.go.dev/github.com/apache/beam/sdks/[email protected]/go/pkg/beam/core/util/hooks#IsEnabled

LeoCBS added 2 commits November 27, 2024 09:59

Adding new entry to CHANGES.md

bfc9077

github-actions bot added go io labels Nov 27, 2024

github-actions bot added the Next Action: Reviewers label Nov 27, 2024

github-actions bot added the slow-review label Dec 5, 2024

github-actions bot removed the slow-review label Dec 9, 2024

lostluck approved these changes Dec 11, 2024

View reviewed changes

github-actions bot added the slow-review label Dec 20, 2024

github-actions bot removed the slow-review label Dec 25, 2024

lostluck requested changes Dec 30, 2024

View reviewed changes

github-actions bot added the slow-review label Jan 7, 2025

github-actions bot removed the slow-review label Jan 10, 2025

github-actions bot added the slow-review label Jan 18, 2025

github-actions bot removed the slow-review label Jan 22, 2025

lostluck added Next Action: Author and removed Next Action: Reviewers labels Feb 13, 2025

github-actions bot added Next Action: Reviewers and removed Next Action: Author labels Feb 16, 2025

github-actions bot added the slow-review label Feb 23, 2025

github-actions bot removed the slow-review label Feb 26, 2025

github-actions bot added Next Action: Author and removed Next Action: Reviewers labels Feb 26, 2025

github-actions bot added Next Action: Reviewers and removed Next Action: Author labels Apr 21, 2025

github-actions bot added the slow-review label Apr 28, 2025

github-actions bot removed the slow-review label May 1, 2025

Merge branch 'apache:master' into master

28f5727

LeoCBS force-pushed the master branch from b2d3613 to 28f5727 Compare May 11, 2025 23:07

LeoCBS commented May 11, 2025

View reviewed changes

lostluck requested changes May 11, 2025

View reviewed changes

LeoCBS force-pushed the master branch from 2517bbb to 8c27b95 Compare May 18, 2025 11:57

Merge branch 'apache:master' into master

ac18b48

LeoCBS force-pushed the master branch from 8c27b95 to ac18b48 Compare May 18, 2025 12:00

LeoCBS requested a review from lostluck May 18, 2025 12:01

Adding Google Storage Requester pays feature to Golang SDK. #33236

Are you sure you want to change the base?

Adding Google Storage Requester pays feature to Golang SDK. #33236

Uh oh!

Conversation

LeoCBS commented Nov 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Nov 27, 2024

Uh oh!

github-actions bot commented Dec 5, 2024

Uh oh!

github-actions bot commented Dec 9, 2024

Uh oh!

codecov bot commented Dec 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

lostluck left a comment

Choose a reason for hiding this comment

Uh oh!

lostluck commented Dec 11, 2024

Uh oh!

LeoCBS commented Dec 12, 2024

Uh oh!

lostluck commented Dec 12, 2024

Uh oh!

github-actions bot commented Dec 20, 2024

Uh oh!

LeoCBS commented Dec 22, 2024

Uh oh!

github-actions bot commented Dec 25, 2024

Uh oh!

lostluck commented Dec 27, 2024

Uh oh!

lostluck left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jan 7, 2025

Uh oh!

github-actions bot commented Jan 10, 2025

Uh oh!

github-actions bot commented Jan 18, 2025

Uh oh!

github-actions bot commented Jan 22, 2025

Uh oh!

lostluck commented Feb 13, 2025

Uh oh!

LeoCBS commented Feb 16, 2025

Uh oh!

github-actions bot commented Feb 23, 2025

Uh oh!

github-actions bot commented Feb 26, 2025

Uh oh!

lostluck commented Feb 26, 2025

Uh oh!

claudevdm commented Apr 15, 2025

Uh oh!

LeoCBS commented Apr 21, 2025

Uh oh!

github-actions bot commented Apr 28, 2025

Uh oh!

github-actions bot commented May 1, 2025

Uh oh!

LeoCBS commented May 11, 2025

Uh oh!

LeoCBS May 11, 2025

Choose a reason for hiding this comment

Uh oh!

lostluck May 11, 2025

Choose a reason for hiding this comment

Uh oh!

lostluck May 11, 2025

Choose a reason for hiding this comment

Uh oh!

LeoCBS May 18, 2025

Choose a reason for hiding this comment

Uh oh!

lostluck May 11, 2025

Choose a reason for hiding this comment

LeoCBS commented Nov 27, 2024 •

edited

Loading

codecov bot commented Dec 11, 2024 •

edited

Loading