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

Improved some plugin parameters #2

Merged
merged 5 commits into from
Oct 25, 2016
Merged

Improved some plugin parameters #2

merged 5 commits into from
Oct 25, 2016

Conversation

Albibek
Copy link

@Albibek Albibek commented Oct 13, 2016

  • (Breaking change) Changed broker address to URI to handle ws:// and other supported schemas
  • Added reconnect parameters and retry counter
  • Added QoS specification via tuple fields

@nobu-k
Copy link
Member

nobu-k commented Oct 13, 2016

Thanks for the PR! I'll merge this after testing the changes. Also, I'll make a few additional small changes: (1) support old "host:port" style for compatibility in addition to the new style (the old style becomes obsolete), (2) fixes for golint if present (I thought the message in Errorf should start with a lowercase letter).

@Albibek
Copy link
Author

Albibek commented Oct 14, 2016

Thanks for answering.
There is one more problem that becomes opened when we enable ssl://urls: TLS options. There is a lot of them. While some of them seem pretty rare to me, some are really the ones we cannot live with. RootCAs and cliet certificte options are the examples of really impotant ones.
I could implement them, but I'd like to hear your opinion about the implementation. Should we do them in a similar way as other sink/source parameters or provide some other, more convenient way to set them all at once?

@nobu-k
Copy link
Member

nobu-k commented Oct 14, 2016

That's a lot :) Because I'm flying back to the US from Japan tomorrow, please let me have time to think of a proposal. I'll probably create a new issue for it and merge this PR first.

@nobu-k
Copy link
Member

nobu-k commented Oct 19, 2016

@Albibek Can the default value of reconnect_retries be -1? It makes the behavior backward compatible.

Actually, I'm not sure if there's any use case that a user want to stop the source when a MQTT broker is down. @tgpfeiffer Do you think this retry counter is necessary?

@Albibek
Copy link
Author

Albibek commented Oct 19, 2016

@nobu-k , you're right, the infinite value is far more reasonable than finite one. I think finite value here could still be meangful in some future cases where it is possible to specify more than one broker.
But it's the future anyways. In current state, how would you like me to fix it - remove it at all or set it to -1?

@nobu-k
Copy link
Member

nobu-k commented Oct 19, 2016

Thanks for the comment. I think we should remove the parameter at the moment and add it again when it gets required. Your usecase makes sense to me, and we'll probably have a better idea on retries when we actually support multiple brokers.

@nobu-k nobu-k mentioned this pull request Oct 19, 2016
@nobu-k
Copy link
Member

nobu-k commented Oct 19, 2016

I can't still think of a good way to support lots of TLS parameters effectively, but I created an issue for it to merge this PR in advance. #3 Could you make some comments on the issue if you already had some idea?

@tgpfeiffer
Copy link
Contributor

Actually, I'm not sure if there's any use case that a user want to stop the source when a MQTT broker is down. @tgpfeiffer Do you think this retry counter is necessary?

I can imagine that, when an MQTT broker goes/is down, both behaviors ("retry forever" and "retry only N times, then give up") may be desired and should be available. As for the default, I think "retry forever" makes more sense.
Not adding the parameter at all (implying "retry forever") is ok for me until we receive some other request.

@Albibek
Copy link
Author

Albibek commented Oct 20, 2016

Well, having the whole sensorbee still working while some sources are stopped and cannot restore their state brings the program to some black-boxed inconsistent state.

Can such state be monitored via REST or any other status report or the user only has logs? Do you think that log reporting is enough here? Does the source have the ability to report it's inconsistent state or just be restarted?

@nobu-k
Copy link
Member

nobu-k commented Oct 20, 2016

Can such state be monitored via REST or any other status report or the user only has logs?

Currently, watching logs are the only way to know if something wrong has happened. I think log reporting isn't enough. Especially, the main purpose of SensorBee is to run stream data processing at the edge of the network and it isn't always possible to watch log files there.

Internally, it is possible to leave the stopped source and see what happened via REST API with error information if any. However, the current implementation automatically removes all stopped nodes and there's no way to configure that behavior. If you need this kind of feature, please give us a request.

@Albibek
Copy link
Author

Albibek commented Oct 21, 2016

Well, it seems to me that reconnect retries value is of no concern to be set to -1. See the commit above.

@Albibek
Copy link
Author

Albibek commented Oct 21, 2016

Considering other parts of discussion.
I was asking about source restoration in terms of reconnect parameter handling. Your answer confirms my concerns about lost mqtt broker. It can become lost completely until sensorbee restart.
What's your final decision about that. Should I remove reconnectRetries at all or leave it, probably with the warning to the user about the case we talked about?

@nobu-k
Copy link
Member

nobu-k commented Oct 21, 2016

Let's remove reconnect_retries parameter for the moment and leave other parts. I think we'll be able to reuse most of your retry implmentation when we support multiple brokers. Also, it's very concise and has little cost to maintain.

I can make that change and merge this PR if you're okay with it.

@Albibek
Copy link
Author

Albibek commented Oct 24, 2016

Done. Thought about the totally same thing.

@nobu-k nobu-k merged commit 6a94b29 into sensorbee:master Oct 25, 2016
@nobu-k
Copy link
Member

nobu-k commented Oct 25, 2016

Thanks!! I merged this PR with following changes: supporting old format of broker parameter, fixes for golint warning including the one that has already existed before this PR.

I'll release v1.1.0.

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.

3 participants