Skip to content

Fix the publish error response #220

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

Jump to bottom
Merged
merged 2 commits into from
Aug 1, 2023
Merged

Fix the publish error response #220

merged 2 commits into from
Aug 1, 2023

Conversation

Zerpet
Copy link
Member

@Zerpet Zerpet commented Jul 28, 2023

Summary

  • Move to uber Gomock
  • Fix publisher error response frame

@Zerpet Zerpet requested review from ablease and removed request for ablease July 28, 2023 16:19
Zerpet added 2 commits July 28, 2023 17:22
@Zerpet
Move to uber Gomock
57339a1 
Golang moved the project to Uber:
https://github.com/uber-go/mock

Signed-off-by: Aitor Pérez Cedres <acedres@vmware.com>
@Zerpet
Fix publisher error response frame
f5e9eea 
Publish error frame returns a list of publishing errors. The caller has
to go through all the errors to determine what publishing IDs failed to
publish. We were not reading all the bytes from the socket, which was
missaligning the buffer, leading to a panic in the handleIncoming
function.

Signed-off-by: Aitor Pérez Cedres <acedres@vmware.com>
@Zerpet Zerpet force-pushed the zerpet/fix-publish-error branch from 183902b to f5e9eea Compare July 28, 2023 16:22
@Gsantomaggio Gsantomaggio added the v2.x Applicable to v2.x issues label Jul 31, 2023
@Gsantomaggio
Copy link
Member

Gsantomaggio commented Aug 1, 2023 •
edited
Loading

make tests-ci works, but there is some problem with make tests. Am I missing something?

• [FAILED] [0.016 seconds]
Environment [DeferCleanup (Each)] delete stream when there are multiple locators uses different locators when one fails [flaky]
  [DeferCleanup (Each)] /Users/gas/go/pkg/mod/go.uber.org/mock@v0.2.0/gomock/controller.go:102
  [It] /Users/gas/go/src/github.com/rabbitmq/rabbitmq-stream-go-client/pkg/stream/environment_test.go:230

  Timeline >>
  [FAILED] in [DeferCleanup (Each)] - /usr/local/go/src/reflect/value.go:584 @ 08/01/23 14:45:24.408

  Attempt #1 Failed.  Retrying ↺ @ 08/01/23 14:45:24.408

  [FAILED] in [DeferCleanup (Each)] - /usr/local/go/src/reflect/value.go:584 @ 08/01/23 14:45:24.413

  Attempt #2 Failed.  Retrying ↺ @ 08/01/23 14:45:24.413

  [FAILED] in [DeferCleanup (Each)] - /usr/local/go/src/reflect/value.go:584 @ 08/01/23 14:45:24.418
  << Timeline

  [FAILED] missing call(s) to *stream.MockRawClient.IsOpen() /Users/gas/go/src/github.com/rabbitmq/rabbitmq-stream-go-client/pkg/stream/environment_test.go:226
  In [DeferCleanup (Each)] at: /usr/local/go/src/reflect/value.go:584 @ 08/01/23 14:45:24.418

  There were additional failures detected.  To view them in detail run ginkgo -vv
------------------------------
↺ [FLAKEY TEST - TOOK 3 ATTEMPTS TO PASS] [0.020 seconds]
Environment query stream stats when there are multiple locators gives up on non-retryable errors [flaky]
/Users/gas/go/src/github.com/rabbitmq/rabbitmq-stream-go-client/pkg/stream/environment_test.go:402
------------------------------
• [FAILED] [0.023 seconds]
Environment delete stream when there are multiple locators [It] gives up on non-retryable errors [flaky]
/Users/gas/go/src/github.com/rabbitmq/rabbitmq-stream-go-client/pkg/stream/environment_test.go:248

  Timeline >>
  [FAILED] in [It] - /Users/gas/go/src/github.com/rabbitmq/rabbitmq-stream-go-client/pkg/stream/locator.go:74 @ 08/01/23 14:45:24.416
  [FAILED] in [DeferCleanup (Each)] - /usr/local/go/src/reflect/value.go:584 @ 08/01/23 14:45:24.419

  Attempt #1 Failed.  Retrying ↺ @ 08/01/23 14:45:24.421

  [FAILED] in [It] - /Users/gas/go/src/github.com/rabbitmq/rabbitmq-stream-go-client/pkg/stream/locator.go:74 @ 08/01/23 14:45:24.425
  [FAILED] in [DeferCleanup (Each)] - /usr/local/go/src/reflect/value.go:584 @ 08/01/23 14:45:24.427

  Attempt #2 Failed.  Retrying ↺ @ 08/01/23 14:45:24.427

  [FAILED] in [It] - /Users/gas/go/src/github.com/rabbitmq/rabbitmq-stream-go-client/pkg/stream/locator.go:74 @ 08/01/23 14:45:24.433
  [FAILED] in [DeferCleanup (Each)] - /usr/local/go/src/reflect/value.go:584 @ 08/01/23 14:45:24.434
  << Timeline

  [FAILED] Unexpected call to *stream.MockRawClient.IsOpen([]) at /Users/gas/go/src/github.com/rabbitmq/rabbitmq-stream-go-client/pkg/stream/locator.go:74 because: there are no expected calls of the method "IsOpen" for that receiver
  In [It] at: /Users/gas/go/src/github.com/rabbitmq/rabbitmq-stream-go-client/pkg/stream/locator.go:74 @ 08/01/23 14:45:24.433

  There were additional failures detected.  To view them in detail run ginkgo -vv
------------------------------

Summarizing 2 Failures:
  [FAIL] Environment [DeferCleanup (Each)] delete stream when there are multiple locators uses different locators when one fails [flaky]
  /usr/local/go/src/reflect/value.go:584
  [FAIL] Environment delete stream when there are multiple locators [It] gives up on non-retryable errors [flaky]
  /Users/gas/go/src/github.com/rabbitmq/rabbitmq-stream-go-client/pkg/stream/locator.go:74

Ran 35 of 36 Specs in 0.065 seconds
FAIL! -- 33 Passed | 2 Failed | 1 Flaked | 0 Pending | 1 Skipped


Ginkgo ran 4 suites in 8.881112987s

There were failures detected in the following suites:
  stream ./pkg/stream

Test Suite Failed
make: *** [tests] Error 1
➜  rabbitmq-stream-go-client git:(zerpet/fix-publish-error) ✗

@Gsantomaggio
Copy link
Member

Gsantomaggio commented Aug 1, 2023 •
edited
Loading

Ok I understood the problem, we filter the flaky tests: --label-filter="!flaky" on the make tests-ci

Let's ignore it for this PR. Maybe we should investigate how to remove the flaky filter in v2 client.

Gsantomaggio
Gsantomaggio approved these changes Aug 1, 2023
View reviewed changes
Copy link
Member

@Gsantomaggio Gsantomaggio left a comment

Choose a reason for hiding this comment

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

Looks good to me. Publish error is not tested in the e2e test btw

@Zerpet
Copy link
Member Author

Zerpet commented Aug 1, 2023

We could add an e2e test to trigger all possible errors from the broker 🤔 I'll create an issue for that

@Zerpet Zerpet merged commit 41c9e8e into v2 Aug 1, 2023
@Zerpet Zerpet deleted the zerpet/fix-publish-error branch August 1, 2023 16:35
@Zerpet Zerpet mentioned this pull request Aug 1, 2023
Closed
@Zerpet Zerpet added this to the 2.0 milestone Aug 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Gsantomaggio Gsantomaggio Gsantomaggio approved these changes

Assignees
No one assigned
Labels
v2.x Applicable to v2.x issues
Projects
None yet
Milestone
2.0
Development

Successfully merging this pull request may close these issues.
2 participants
@Zerpet @Gsantomaggio