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

Cannot create_bucket with queued=True #37

Open
Otto-AA opened this issue Jul 3, 2018 · 11 comments
Open

Cannot create_bucket with queued=True #37

Otto-AA opened this issue Jul 3, 2018 · 11 comments

Comments

@Otto-AA
Copy link

Otto-AA commented Jul 3, 2018

When using queued=True the client doesn't send the create_bucket request to the server.

Steps to reproduce:

  1. Start aw-server in testing mode: aw-server --testing --verbose

  2. Run following python script:

from aw_client import ActivityWatchClient
client = ActivityWatchClient("test-client", True)
client.create_bucket('test-bucket_{}'.format(client.hostname), event_type='test-event-type', queued=True)

Expected behaviour

A bucket is created.

Context

  • Ubuntu 18.04 LTS
  • python3
  • Versions: unknown (can look up if necessary)
@ErikBjare
Copy link
Member

You need to run client.connect() (and give it some time to create the bucket) or use the context manager: with client: ...

Feel free to improve the documentation if you found it lacking.

@Otto-AA
Copy link
Author

Otto-AA commented Jul 3, 2018

Documentation is pretty good, seems like I've just skipped this part -_-

Nonetheless, what do you think about a warning in such a case? Or is this something that is intended for some scenarios?

@Otto-AA
Copy link
Author

Otto-AA commented Jul 3, 2018

And is client.disconnect() mandatory or can I just let the program exit without calling it?

@johan-bjareholt
Copy link
Member

Nonetheless, what do you think about a warning in such a case? Or is this something that is intended for some scenarios?

Well, where should the warning go? IMO it should be fine to start queueing requests before connecting so I don't think that's the right time.

And is client.disconnect() mandatory or can I just let the program exit without calling it?

Nothing will break, but the last event might be lost otherwise so it's good practice. Best way IMO is to use with client: ... as @ErikBjare said

@ErikBjare
Copy link
Member

@Otto-AA I'd be in favor of omitting a warning, limiting it to once per run and disabling it by setting a kwarg to True explicitly should be fine.

What do you think @johan-bjareholt?

And yes, on further thought with client: ... is definitely the way to go. If we show normal use of connect() and disconnect() in the docs we should definitely change them.

@johan-bjareholt
Copy link
Member

and disabling it by setting a kwarg to True explicitly should be fine.

I don't agree with this, setting a kwarg only to disable a warning message is dirty IMO.

@ErikBjare
Copy link
Member

ErikBjare commented Jul 3, 2018

@johan-bjareholt It should be really rare to not want the queue active if adding queued events or creating a queued bucket, and can understandably lead to confusion if one hasn't read the docs.

There are other ways to ignore warnings here. Which do you prefer?

@Otto-AA
Copy link
Author

Otto-AA commented Jul 3, 2018

Using with client seems hard to do, as I store it in the config file so it can be shared across main.py and message_handler.py

I can do it with atexit.register(clean_up) though.

@johan-bjareholt
Copy link
Member

@ErikBjare Well, in that case I think we should just make a deprecated warning every time we queue before connecting and later disable it completely.

However, it is currently unsupported to create a bucket after the RequestQueue has connected I believe so we have to fix that first (It's really stupid, I know but that's how it is due to how create_bucket previously was named setup_bucket but never changed behaviour to reflect that change). An extra call to self._create_buckets upon register_bucket should suffice.

There's not really any benefit of creating buckets before connecting though, it's just more flexible to allow both. It is just the kwarg silencing I'm against, the idea is fine otherwise IMO and should fix this confusion.

@ErikBjare
Copy link
Member

ErikBjare commented Jul 4, 2018

@Otto-AA What's hard about with client: ...? You should still be able to share it across files however you want, I don't see the issue.

So, @johan-bjareholt, is this right then?

  • Support calling create_bucket() after connect()
    • An extra call to self._create_buckets upon register_bucket should suffice.
  • Warn when create_bucket(*args, queued=True) or heartbeat(*args, queued=True) is called before connect()

@johan-bjareholt
Copy link
Member

Support calling create_bucket() before connect()

No, support calling create_bucket() AFTER connect() :p

Preferably also add a TODO comment to make it impossible rather than just a warning in the future

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

No branches or pull requests

3 participants