[
https://issues.apache.org/jira/browse/BEAM-4424?focusedWorklogId=665263&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-665263
]
ASF GitHub Bot logged work on BEAM-4424:
----------------------------------------
Author: ASF GitHub Bot
Created on: 13/Oct/21 19:00
Start Date: 13/Oct/21 19:00
Worklog Time Spent: 10m
Work Description: lostluck commented on a change in pull request #15713:
URL: https://github.com/apache/beam/pull/15713#discussion_r728253180
##########
File path: sdks/go/pkg/beam/core/runtime/harness/harness.go
##########
@@ -47,8 +47,10 @@ const cacheSize = 20
func Main(ctx context.Context, loggingEndpoint, controlEndpoint string) error {
hooks.DeserializeHooksFromOptions(ctx)
+ // Pass in the logging endpoint for use w/the default remote logging
hook.
+ ctx = context.WithValue(ctx, loggingEndpointCtxKey, loggingEndpoint)
Review comment:
Yes. An alternative would be to move the remote logging set up to the
harness/init package, so the flag is plumbed as a flag instead of as the
context. That would require exporting some of the logging set up so it would be
accessible though, as we don't want to copy the GRPC code out of the harness
package.
Overall, this was the least obtrusive. At worst, the context is carrying
around an extra value, but this is fine.
##########
File path: sdks/go/pkg/beam/core/runtime/harness/logging.go
##########
@@ -95,6 +96,28 @@ func convertSeverity(sev log.Severity)
fnpb.LogEntry_Severity_Enum {
}
}
+type remoteLoggingKey string
+
+// DefaultRemoteLoggingHook is the key used for the default remote logging
hook.
+// If a runner wants to use an alternative logging solution, it can be
+// disabled with hooks.DisableHook(harness.DefaultRemoteLoggingHook).
+const DefaultRemoteLoggingHook = "default_remote_logging"
+
+var loggingEndpointCtxKey = remoteLoggingKey(DefaultRemoteLoggingHook)
+
+func init() {
Review comment:
That's right, this sets up remote logging to the FnAPI Logging Service,
for the runner to do with as it likes.
That's the correct flow.
Since all beam Go workers depend on the harness, this init happens pretty
early in the package initialization process, and likely before any user code.
Certainly before any runner code (the harness dependency happens through the
runners). That means the runners can disable it at their leisure.
The alternative would be to register the hook and have runners enable it
explicitly, however, this is more likely to be missed by newer runner packages.
Simpler to have the default work as expected, and have exceptional runners that
want to make different choices explicitly disable the normal log handling.
That's correct, DeserilaizeHooksFromOptions overwrites all the existing
hooks in the registry. The function is only happening at worker side anyway at
harness main, and at that point we need the hooks to be relayed through the
options.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Issue Time Tracking
-------------------
Worklog Id: (was: 665263)
Time Spent: 2h 10m (was: 2h)
> Improvements to hooks module
> ----------------------------
>
> Key: BEAM-4424
> URL: https://issues.apache.org/jira/browse/BEAM-4424
> Project: Beam
> Issue Type: Improvement
> Components: sdk-go
> Affects Versions: Not applicable
> Reporter: Bill Neubauer
> Assignee: Robert Burke
> Priority: P3
> Time Spent: 2h 10m
> Remaining Estimate: 0h
>
> Proposed improvements to the Go hooks API:
> Execution order of the hooks should be based on the order in which the hook
> is enabled. This gives the runner precise control over ordering so
> dependencies on hooked behavior can be well-managed.
> Provide an API to disable a hook. Disabling a hook removes it from the
> ordered list. If the same hook is later re-Enabled, it has lost its previous
> ordering and would be placed at the end of the ordered list.
> The invocation of setupRemoteLogging() in harness.Main() will be replaced by
> a hook. This new hook will be called by the default translate code. The net
> effect is the default behavior for runners remains unchanged. If a runner
> wants a different logging behavior, it can disable the default logging hook
> and enable its own hook.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
