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.

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

Sporadic github action build failure fix #522

Merged
merged 23 commits into from
Oct 23, 2023
Merged

Sporadic github action build failure fix #522

merged 23 commits into from
Oct 23, 2023

Conversation

akrambek
Copy link
Contributor

@akrambek akrambek commented Oct 18, 2023

Description

  • Draining remaining frames too late causes a timeout
  • Replacing supplyLabelId with NamespacedId.localId
  • Race in shouldReceiveMessagesWithIsolationReadUncommittedWhenAborted scripts

Fixes #526

@akrambek akrambek changed the title Build failures Sporadic build failure fix Oct 20, 2023
@akrambek akrambek changed the title Sporadic build failure fix Sporadic github action build failure fix Oct 20, 2023
@akrambek akrambek marked this pull request as ready for review October 20, 2023 23:12
@@ -282,6 +289,11 @@ public void close() throws Exception
}
}

private void drain()
{
dispatchers.forEach(d -> d.drain());
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
dispatchers.forEach(d -> d.drain());
dispatchers.forEach(DispatchAgent::drain);

Copy link
Contributor

Choose a reason for hiding this comment

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

Also suggest we just inline this, since we don't need a separate Engine.drain() method.

@@ -90,6 +90,8 @@ read zilla:data.ext ${kafka:matchDataEx()
.build()}
read "Hello, world"

read notify SENT_ABORT

Copy link
Contributor

Choose a reason for hiding this comment

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

Normally we think of barriers as events that need to be notified because they happened, rather than naming something yet to happen in the future.

So we should rename this to reflect what has already happened here.

Suggest something like RECEIVED_SKIPPED_MESSAGE?

@@ -73,6 +73,8 @@ write zilla:begin.ext ${kafka:beginEx()
.build()}
write flush

write await SENT_ABORT

Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be between partition(0, 3, 3, 3) and partition(0, 4, 4, 4) instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if I understood you correctly, it is between those two message

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't mean the beginEx, referring to between the flushEx and the dataEx.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

test are failing if I put between the flushEx and the dataEx.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, makes sense - the barrier needs to go before the flush.

@@ -73,6 +73,8 @@ write zilla:begin.ext ${kafka:beginEx()
.build()}
write flush

write await SENT_ABORT

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't mean the beginEx, referring to between the flushEx and the dataEx.

jfallows
jfallows previously approved these changes Oct 23, 2023
@@ -73,6 +73,8 @@ write zilla:begin.ext ${kafka:beginEx()
.build()}
write flush

write await SENT_ABORT

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, makes sense - the barrier needs to go before the flush.

@jfallows jfallows merged commit cd59556 into aklivity:develop Oct 23, 2023
ankitk-me pushed a commit to ankitk-me/zilla that referenced this pull request Oct 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sporadic github action build failures
2 participants