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

Experimental quorum queues #19

Merged
merged 20 commits into from
Feb 16, 2021
Merged

Experimental quorum queues #19

merged 20 commits into from
Feb 16, 2021

Conversation

phijor
Copy link
Contributor

@phijor phijor commented Jan 8, 2021

This pull request has kind of outgrown its orignial scope.

What this does:

  • Unify configuration parsing, by adding a new ConfigParser class
  • Manage all queue operations in a central place, the QueueManager.
  • Make queue operations less likely to leave the manager in a "zombie state" by using temporary channels. (961c053)
  • Allow the use of quorum queues, configurable per-client and per-queue.

TODO:

  • document manager, especially how client configuration is parsed and translated into queue setup (2fc0eda, 7f82e03)
  • move config into x-metricq subkey (see ConfigParser.top_level_key)
  • check that having dashes in config keys (-) are not a problem
    • yep, that's explicitly allowed: the receiver must have a catch-all **kwargs parameter:

      def config(foo, bar, **kwargs):
          assert "with-dash" in kwargs
      
      config(1, 2, **{"with-dash": True})
  • do not close temporary channels in background task (here). Await channel closure directly, but log and discard errors that happen when closing. (part of 961c053)
  • assign queue names that contain the queue type, like {token}[-{uuid}]-{queue_type}-{role}. This way, changing the queue type and re-declaring a client's queue does not result in errors because of queue argument mismatches. The old queue needs to be deleted by hand though afterwards. (c4ea672)

@phijor phijor force-pushed the experimental-quorum-queues branch 2 times, most recently from 8296de9 to 7468533 Compare January 21, 2021 19:47
@phijor phijor force-pushed the experimental-quorum-queues branch 2 times, most recently from 3a03e20 to 2ffad69 Compare January 26, 2021 12:57
setup.cfg Outdated

[metadata]
name = metricq_manager
version = 0.1
Copy link
Member

Choose a reason for hiding this comment

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

we should increase the version for this.

metricq_manager/config_parser.py Outdated Show resolved Hide resolved
metricq_manager/config_parser.py Outdated Show resolved Hide resolved
metricq_manager/config_parser.py Outdated Show resolved Hide resolved
metricq_manager/config_parser.py Show resolved Hide resolved
metricq_manager/queue_manager.py Outdated Show resolved Hide resolved
metricq_manager/queue_manager.py Show resolved Hide resolved
metricq_manager/queue_manager.py Show resolved Hide resolved
metricq_manager/queue_manager.py Outdated Show resolved Hide resolved
tools/db-predeclare.py Show resolved Hide resolved
Copy link
Member

@bmario bmario 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, but I found one typo :)

metricq_manager/manager.py Outdated Show resolved Hide resolved
When changing the queue type of a client queue, its next declaration is
going to fail with error code 406 (Precondition failed), since it does
not match the arguments of the existing queue.

We include the queue type in the queue name to sidestep the problem.
Changing the queue type then results in a new queue (with a predictable
name) being declared and no conflicts arise.  A client re-registering
itself with a changed queue type then gets assigned the newly declared
queue.  Of course, the old queue still exists and needs to be deleted
manually.

For compatibility with the old code, wee keep queue old names for
default queue type.
By using a temporary channel for each operation, failures do not impact
later queue operations.  This prevents the dreaded "zombie state" where
`ChannelClosed` errors appear for every queue operation, after one
operation failed.

When closing the temporary channel, all related AMQP exceptions are
logged and then discarded.
… method

This is related to #17: once every Database explicitly calls
`db.subscribe` after registering itself, this method should be removed.
Then, `handle_db_register` should simply declare the DB queues without
any bindings.
Methods should have a name of `{client-type}_{operation}`.
Explain how to use the temporary_channel context manger correctly [1].

For exchanges, aio_pika.Queue.bind() also accepts an exchange name
instead of an Exchange object, so after declaring the exchanges we only
save their names and use those to bind metrics.

For queues, make sure to always retrieve the queue name before dropping
the associated channel.  No instance of aio_pika.Queue should ever leave
a `async with self.temporary_channel():`-block.

[1] https://www.urbandictionary.com/define.php?term=You%27re%20Holding%20It%20Wrong
@phijor
Copy link
Contributor Author

phijor commented Feb 16, 2021

Rebased and ready to merge :)

@phijor phijor merged commit 0b12201 into master Feb 16, 2021
@bmario bmario deleted the experimental-quorum-queues branch February 16, 2021 12:44
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