Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Trigger_Never to Prepare features check #31472

Merged
merged 3 commits into from
Jun 3, 2024
Merged

Add Trigger_Never to Prepare features check #31472

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a980b5a
Add Trigger_Never to Prepare features check
damondouglas Jun 1, 2024
c16d1e9
Remove TriggerNever from TestUnimplemented
damondouglas Jun 1, 2024
fd6fc36
Add TriggerNever to TestImplemented
damondouglas Jun 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion sdks/go/pkg/beam/runners/prism/internal/jobservices/management.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,7 +201,13 @@ func (s *Server) Prepare(ctx context.Context, req *jobpb.PrepareJobRequest) (*jo
check("WindowingStrategy.OutputTime", ws.GetOutputTime(), pipepb.OutputTime_END_OF_WINDOW)
// Non nil triggers should fail.
if ws.GetTrigger().GetDefault() == nil {
check("WindowingStrategy.Trigger", ws.GetTrigger(), &pipepb.Trigger_Default{})
dt := &pipepb.Trigger{
Trigger: &pipepb.Trigger_Default_{},
}
nt := &pipepb.Trigger{
Trigger: &pipepb.Trigger_Never_{},
}
check("WindowingStrategy.Trigger", ws.GetTrigger().String(), dt.String(), nt.String())
Comment on lines +204 to +210
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The original ws.GetTrigger() returns a *pipepb.Trigger which would have never passed == &pipepb.Trigger_Default{}. In order for the existing check to evaluate equality would have needed cmp.Equals. Checking the String() was easier and still achieves the desired outcome.

}
}
}
Expand Down
1 change: 0 additions & 1 deletion sdks/go/pkg/beam/runners/prism/internal/unimplemented_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,6 @@ func TestUnimplemented(t *testing.T) {
{pipeline: primitives.TriggerAfterProcessingTime},
{pipeline: primitives.TriggerAfterSynchronizedProcessingTime},
{pipeline: primitives.TriggerElementCount},
{pipeline: primitives.TriggerNever},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since this is default, does it make sense to remove this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Tests removed from this list must be added to another test that expects them to pass. Otherwise we're opening a test regression gap for ourselves down the road.

So, if you're removing this because TestUnimplmented started to fail, then you must move it to the TestImplmenented list down below.

If this test didn't start to fail, then we should leave it here to show we aren't fully done work on TriggerNever.

{pipeline: primitives.TriggerOrFinally},
{pipeline: primitives.TriggerRepeat},

Expand Down
Loading