aw-client icon indicating copy to clipboard operation
aw-client copied to clipboard

Cannot create_bucket with queued=True

Open Otto-AA opened this issue 7 years ago • 11 comments

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)

Otto-AA avatar Jul 03 '18 15:07 Otto-AA

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.

ErikBjare avatar Jul 03 '18 15:07 ErikBjare

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 avatar Jul 03 '18 16:07 Otto-AA

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

Otto-AA avatar Jul 03 '18 16:07 Otto-AA

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

johan-bjareholt avatar Jul 03 '18 19:07 johan-bjareholt

@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.

ErikBjare avatar Jul 03 '18 19:07 ErikBjare

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.

johan-bjareholt avatar Jul 03 '18 19:07 johan-bjareholt

@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?

ErikBjare avatar Jul 03 '18 19:07 ErikBjare

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.

Otto-AA avatar Jul 03 '18 21:07 Otto-AA

@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.

johan-bjareholt avatar Jul 04 '18 08:07 johan-bjareholt

@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()

ErikBjare avatar Jul 04 '18 10:07 ErikBjare

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

johan-bjareholt avatar Jul 04 '18 11:07 johan-bjareholt