[pinpoint] retain swappability of catapult bisect workflow
The response object was updated to a new struct object that contains
the datastore response, but this makes the response type different from
internal.Bisect and makes this workflow less interchangeable with it.
The internal.BisectExecution object currently returns a good amount
of additional data to generate the information for Catapult. However,
it being under the internal package doesn't allow it to be exposed.
Anomaly workflows need access to Culprits.
To address both, response type is updated to pinpoint_proto, which
contains just the information about culprit while being accessible
outside of internal. Once the backwards compatibility is no longer
needed, the response object of internal.BisectWorkflow can be
reverted to the pinpoint_proto and the catapult package can be
deleted.
Bug: b/327032048
Change-Id: I2b88c9658a3db285dfb44a19f53afcd779d05f2c
Reviewed-on: https://skia-review.googlesource.com/c/buildbot/+/848487
Commit-Queue: Jeff Yoon <jeffyoon@google.com>
Reviewed-by: Prakhar Asthana <pasthana@google.com>
Reviewed-by: Leina Sun <sunxiaodi@google.com>
diff --git a/pinpoint/go/workflows/catapult/catapult_bisect.go b/pinpoint/go/workflows/catapult/catapult_bisect.go
index e333b32..eae5c20 100644
--- a/pinpoint/go/workflows/catapult/catapult_bisect.go
+++ b/pinpoint/go/workflows/catapult/catapult_bisect.go
@@ -17,11 +17,6 @@
BisectJobNameTemplate = "[Skia] Performance bisect on %s/%s"
)
-type CatapultBisectResponse struct {
- *internal.BisectExecution
- *DatastoreResponse
-}
-
// updateStatesWithComparisons goes through each state and appends legacy comparisons
func updateStatesWithComparisons(states []*pinpoint_proto.LegacyJobResponse_State, magnitude float64, direction compare.ImprovementDir) error {
if len(states) < 2 {
@@ -118,9 +113,10 @@
// the responses to Catapult via Skia-Bridge, such that the Catapult UI can display the results.
// Thus, the workflow method signature should be identical to internal.BisectWorkflow.
// This is written in its own package and in its own workflow so that it's self-contained.
-func CatapultBisectWorkflow(ctx workflow.Context, p *workflows.BisectParams) (*CatapultBisectResponse, error) {
+func CatapultBisectWorkflow(ctx workflow.Context, p *workflows.BisectParams) (*pinpoint_proto.BisectExecution, error) {
ctx = workflow.WithChildOptions(ctx, childWorkflowOptions)
ctx = workflow.WithActivityOptions(ctx, regularActivityOptions)
+ logger := workflow.GetLogger(ctx)
var bisectExecution *internal.BisectExecution
if err := workflow.ExecuteChildWorkflow(ctx, internal.BisectWorkflow, p).Get(ctx, &bisectExecution); err != nil {
@@ -137,8 +133,10 @@
return nil, skerr.Wrap(err)
}
- return &CatapultBisectResponse{
- BisectExecution: bisectExecution,
- DatastoreResponse: dsResp,
+ logger.Info(fmt.Sprintf("Datastore information for this job: %v", dsResp))
+
+ return &pinpoint_proto.BisectExecution{
+ JobId: bisectExecution.JobId,
+ Culprits: bisectExecution.Culprits,
}, nil
}
diff --git a/pinpoint/go/workflows/catapult/catapult_bisect_test.go b/pinpoint/go/workflows/catapult/catapult_bisect_test.go
index c6eb75b..ee797fd 100644
--- a/pinpoint/go/workflows/catapult/catapult_bisect_test.go
+++ b/pinpoint/go/workflows/catapult/catapult_bisect_test.go
@@ -82,11 +82,10 @@
require.True(t, env.IsWorkflowCompleted())
require.NoError(t, env.GetWorkflowError())
- var actual *CatapultBisectResponse
+ var actual *pinpoint_proto.BisectExecution
require.NoError(t, env.GetWorkflowResult(&actual))
assert.NotNil(t, actual)
- assert.Equal(t, mockJobId, actual.BisectExecution.JobId)
- assert.Empty(t, actual.BisectExecution.Culprits)
- assert.Equal(t, mockDSResp, actual.DatastoreResponse)
+ assert.Equal(t, mockJobId, actual.JobId)
+ assert.Empty(t, actual.Culprits)
env.AssertExpectations(t)
}
