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

Add QueueType model #946

Merged
merged 2 commits into from
Jun 4, 2024
Merged

Add QueueType model #946

merged 2 commits into from
Jun 4, 2024

Conversation

geirolz
Copy link
Collaborator

@geirolz geirolz commented May 2, 2024

This PR simply adds a model to type the possible queue types.
It also add some useful argument keys.

I was tempted to add the queue type as parameter of DeclarationQueueConfig with default as Classic.
The problem is that semantically, since it is part of the configuration, we should override the the args and this creates breaking changes.
We could consider to let the user override this config like this

    override def declareQueue(channel: AMQPChannel, config: DeclarationQueueConfig): F[Unit] =
      Sync[F].blocking {
        channel.value.queueDeclare(
          config.queueName.value,
          config.durable.isTrue,
          config.exclusive.isTrue,
          config.autoDelete.isTrue,
          Map(`x-queue-type` -> config.queueType.asString) ++ config.arguments
        )
      }.void

@matejcerny
Copy link
Collaborator

I would also add it to the DeclarationQueueConfig, but without the default value. I am fine with the breaking change :) Just add the Classic to DeclarationQueueConfig.default

@geirolz
Copy link
Collaborator Author

geirolz commented May 30, 2024

I would also add it to the DeclarationQueueConfig, but without the default value. I am fine with the breaking change :) Just add the Classic to DeclarationQueueConfig.default

What if the user set a queue type in the args ? I mean if we override the value we introduce a breaking change

// without these changes - declared as quorum
.declareQueue(
  DeclarationQueueConfig
      .default(QueueName("QUEUE"))
      .copy(arguments = Map("x-queue-type" -> "quorum"))
)

// with these changes - declared as classic since we override the value
.declareQueue(
  DeclarationQueueConfig
      .default(QueueName("QUEUE"))
      .copy(arguments = Map("x-queue-type" -> "quorum"))
)

// with these changes - inconsistent
.declareQueue(
  DeclarationQueueConfig
      .default(QueueName("QUEUE"))
      .copy(
         queueType = QueueType.Classic,
         // imagine args passed as variable for instance
         arguments = Map("x-queue-type" -> "quorum")
      )
)

It isn't trivial to keep args open but synced with the queueType field

@matejcerny
Copy link
Collaborator

matejcerny commented May 31, 2024

Oh, I can see it now, thanks for the clarification. In that case, do we event want to introduce this explicit implementation/configuration? The other config types corresponds almost exclusively with the underlying java library and queue type is only implemented via args there (if I am not mistaken). Maybe some helper enum which will just wrap ("x-queue-type" -> "quorum") can be sufficient? What do you think?

@matejcerny
Copy link
Collaborator

Something like:

sealed trait QueueType extends Product with Serializable {
    def asString: String = this match {
      case QueueType.Classic => "classic"
      case QueueType.Quorum => "quorum"
      case QueueType.Stream => "stream"
    }

  def argument: (String, String) = "x-queue-type" -> asString
}

@geirolz
Copy link
Collaborator Author

geirolz commented May 31, 2024

queue type is only implemented via args

Correct

Yes, unfortunately I think the best solution is just to let the user specify the args.
Eventually we could provide 3 new smart constructor for DeclarationQueueConfig to build the definition with this arg

DeclarationQueueConfig.classic()
DeclarationQueueConfig.quorum()
DeclarationQueueConfig.stream()

for sure you can then override the args and invalidate this semantic.

Not having the queue type explicit is IMH bad for usage purpose because you expect a way to declare the type of the queue.
Having just the QueueType.asArgument means that you have to know that that class and method exists, not much convenient I think.

We could treat the QueueType as optional.
And when we declare the queue in Declaration if we have both QueueType and an arg for x-queue-type we raise an error. In this approach .default() set it to None
What do you think ?

@matejcerny
Copy link
Collaborator

Having separate constructors is definitely a good idea. I am just not a fan of raising errors, it would be better to somehow bake it in the domain. But it's tough when the underlying library looks like it looks like.

Not having the queue type explicit is IMH bad for usage purpose

Understand, but if you think about it, this decision (of bad UX) was made by the RabbitMQ team and it's well documented.

Anyway, I don't want to block it, so feel free to make the decision :)

@geirolz
Copy link
Collaborator Author

geirolz commented May 31, 2024

this decision (of bad UX) was made by the RabbitMQ team and it's well documented

Idk, I have the feeling that this way to declare the queue type is something born after. At the beginning there was just classic then they added the quorum and the stream and this is a way to maintain the backward compatibility.

If you look the exchange declaration is similar to what we are trying to achieve here.

I am just not a fan of raising errors

I'd prefer block inconsistency at the beginning to be honest. I'm updating the PR so maybe it's clearer what I mean

@geirolz geirolz merged commit 0d5f70d into profunktor:master Jun 4, 2024
3 checks passed
@geirolz geirolz deleted the add-queue-type branch June 4, 2024 10:18
@matejcerny
Copy link
Collaborator

matejcerny commented Jun 4, 2024

Time for a release? @geirolz Do you have rights to do it?

I also have another PR (#956) maybe we could check it before making another release

@geirolz
Copy link
Collaborator Author

geirolz commented Jun 5, 2024

@matejcerny I've released your draft PR but you forgot to set the proper tag and I forgot to check so now we have 2 releases 5.1.3 and 5.2.0 that are equals, sorry. Not a big deal. The tag was 5.1.3 but the title was 5.2.0 unfortunately

Do you have discord or X for further direct communications ?

@matejcerny
Copy link
Collaborator

matejcerny commented Jun 5, 2024

@geirolz My X is at matej_cerny.

The draft release was made by a draft job, I've just changed the title as by default only the patch version is incremented. But its fine, I will delete the 5.1.3

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.

2 participants