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

TRAF. 1451 add ack broker and remove dingo #16

Open
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

paupm
Copy link
Contributor

@paupm paupm commented Jan 31, 2022

Right now multiple middleware can be attached to homing-pigeon before the output is written; in order to let us transform the input somehow.

This PR is to provide homing-pigeon with an option to stream all ACK/NACK messages to a subscribeable channel.
Also I've removed Dingo since it's causing the build to fail.

docker-compose.yaml Outdated Show resolved Hide resolved
pkg/ack/manager.go Outdated Show resolved Hide resolved
pkg/ack/manager.go Outdated Show resolved Hide resolved
pkg/main.go Outdated Show resolved Hide resolved
pkg/main.go Outdated Show resolved Hide resolved
pkg/ack/manager.go Outdated Show resolved Hide resolved
pkg/readers/reader.go Show resolved Hide resolved
pkg/writers/writer.go Show resolved Hide resolved
pkg/readers/adapters/amqp.go Show resolved Hide resolved
pkg/readers/reader.go Show resolved Hide resolved
pkg/writers/writer.go Show resolved Hide resolved
README.md Show resolved Hide resolved
pkg/main.go Outdated Show resolved Hide resolved
pkg/readers/adapters/amqp.go Show resolved Hide resolved
pkg/main.go Outdated Show resolved Hide resolved
pkg/main.go Outdated Show resolved Hide resolved

go reader.Start()
go middleware.Start()
go incomingMiddleware.HandleInput()
Copy link
Contributor

Choose a reason for hiding this comment

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

I think there is a misconception here. The idea is that middlewareManager is agnostic. It doesn't know about input/output. Just channgels, so you can use that piece as inputMiddlwareManager or outputMiddlewareManager and both have the ability to process messages as middleware or like async GRPC. Now it is a non-generic piece that has two entrypoints that do two different things and does not generalize.

It'd produce two new env variables:
REQUEST_MIDDLEWARES_SOCKET
REQUEST_GRPC_MIDDLEWARES_SOCKET

RESPONSE_MIDDLEWARES_SOCKET
RESPONSE_GRPC_MIDDLEWARES_SOCKET

That it is just the configuration for the same class in two configured objects.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I understand the idea of making middleware manager agnostic to input/output communication. But I didn't find a way to make it more generic, since the channels are typed. The communication between Reader and Writer is through Message type channels, while the communication Writer -> Reader is through Ack type channels.
The name incomingMiddleware here is wrong, it might be middlewareManager.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can use a generic type like interface{}

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have some news about this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, we don't. The task TRAF-1354 was pushed back to the backlog due to new priorities and not enough resources.
This subtask of adding middleware between writer and reader was paused, as does not bring value by itself.

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.

4 participants