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

refactor CliMulti to allow asynchronous curl requests (for those that need to use curl) #5844

Closed
diosmosis opened this issue Jul 16, 2014 · 4 comments
Labels
Bug For errors / faults / flaws / inconsistencies etc. wontfix If you can reproduce this issue, please reopen the issue or create a new one describing it.
Milestone

Comments

@diosmosis
Copy link
Member

The old asynchronous curl request code that was used before CliMulti and related classes were added is gone. If users want to switch to curl requests, we shouldn't use synchronous curl requests. CliMulti should be refactored to allow asynchronous curl requests.

@mattab
Copy link
Member

mattab commented Jul 16, 2014

I vote "wont fix". Users who need good performance should enable CLI support.

we added the "Cli support of Processes" to the system check so users know whether their piwik setup has CLI support or not.

@mattab mattab closed this as completed Jul 16, 2014
@mattab mattab added this to the 2.5.0 - Piwik 2.5.0 milestone Jul 16, 2014
@diosmosis
Copy link
Member Author

This isn't for people who don't have support for CliMulti, it's for people who experience bugs w/ it and don't want to wait until we can reproduce it. Falling back on synchronous curl requests for weeks or potentially months seems like terrible UX. And I can't imagine the speed benefits from using processes instead of HTTP requests will be all that great (if even noticeable) for users whose visits number in the millions and billions where the bulk of the archiving process consists of aggregation.

@mattab
Copy link
Member

mattab commented Jul 16, 2014

If there is an issue or bug with CliMulti we want to fix it asap. it's not terrible UX to fallback to Curl, it just works (but is slower & with its own problems such as HTTP timeouts etc.). We decided few weeks back to remove support of async curl because code was quite ugly and hard to understand. Also we push for CliMulti going forward and are committed to make it super stable. If users don't want to wait that we reproduce, they should send us SSH/FTP access 👍

@diosmosis
Copy link
Member Author

I think it's terrible UX to fall back to synchronous curl requests. And it wasn't just the async curl code that was ugly, it was the entire CronArchive class. In fact, I remember that the bulk of the async curl code was in just one method. It would not be difficult to refactor the CliMulti code to allow async curl requests and would create an opportunity make the code clearer (I wouldn't call the current code pretty).

This is the last I'll post on the topic.

@mattab mattab added the Bug For errors / faults / flaws / inconsistencies etc. label Sep 20, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug For errors / faults / flaws / inconsistencies etc. wontfix If you can reproduce this issue, please reopen the issue or create a new one describing it.
Projects
None yet
Development

No branches or pull requests

2 participants