<div dir="ltr">Thank you for writing these points so clearly. I was working from the other requirements you wrote [2], but it mostly does the ones below. I'll try to give answers inline.<br><br>[2]: <span class="gmail-"><a href="https://pulp.plan.io/issues/2951#note-17" rel="noreferrer" target="_blank">https://pulp.plan.io/issues/<wbr>2951#note-17</a></span><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Aug 24, 2017 at 4:58 PM, Jeff Ortel <span dir="ltr"><<a href="mailto:jortel@redhat.com" target="_blank">jortel@redhat.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span class="gmail-"><br>
<br>
On 08/24/2017 09:51 AM, Brian Bouterse wrote:<br>
> The next step in considering the asyncio downloaders is done and ready to be looked at. The PR made [0]<br>
> replaces the existing downloaders and is "merge-ready". The best way to read about them is through their docs<br>
> [1]. Here are some of its highlights:<br>
><br>
> # Code size and docs<br>
> * huge code savings by switching to asyncio. removes 2427 lines and adds 558 lines.<br>
<br>
</span>Replacing concurrent.futures with asyncio would remove a fair amount of code in batch.py but the remainder of<br>
lines of code removed discarded features. Each can be correlated to use cases [2].<br>
<br>
- Custom error recovery.<br>
Supported recoverable error recovery. <br></blockquote><div>If an exception is raised in a co-routine it propagates up to allow for
custom handling. Meanwhile you can re-enter the coroutine to allow all
other non-errored downloading can occur.<br><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Supported unavailable mirrors. </blockquote><div>Was mirroring in the previous code? I didn't see it. This can be accomplished by subclassing a HttpDownloader and implementing 1 method. I can show an example if that helps. Mirroring is content specific so I think we can't offer this in the plugin API anyway.<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
- Event notifications.<br></blockquote><div>I can see how the threaded design needs one, but with asyncio you don't need one. All of the use cases are done as needed, easily either in core or by plugin writers. The main goal with this design is that its highly extensible to add custom behaviors either in core or a plugin with little extra work. All Python developers know how to subclass things to customize behaviors, but not all of them know how to work with a custom event system. This is one of the main reasons to standardize on a common technology.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Supported streamer forwarding of HTTP headers.<br>
Supported ChangeSet knowing when an artifact was actually downloaded.<br>
Supported calculating digests and size needed ONLY for creating Artifacts.<br>
Supported (future) progress reporting when desired.<br>
Supported (future) mirror lists.<br>
<br>
- Custom Writers - handling of downloaded bits.<br>
Supported writing files to disk.<br></blockquote><div>^ is the default behavior<br> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Supported the streamer streaming downloaded bit stream in the Twisted response instead of writing<br>
the file to disk.<br></blockquote><div>We didn't make any changes to the streamer code so this is unrelated. When we do go to change it though, we can probably update the streamer to work with this in a day. Swapping out the write-to-disk behavior with a stream via twisted behavior is a single method to override in a subclass.<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
- File URLs.<br>
supported: "file:///myfile"<br></blockquote><div>^ This can be easily added. Is that something I should do? I think it would be another 100 lines or so because asyncio does most of the work.<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
- Custom validations without having to subclass the downloader.<br>
Included: digest and size validations but plugin writer could implement additional validations.<br></blockquote><div>It validates size and digests currently. Adding custom validations is easy. Doing it without subclassing is not something the plugin writer really cares about, I think. We didn't talk about that requirement.<br><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
- Standardized settings for SSL and Auth.<br>
Supported consistent settings across downloader types.<br></blockquote><div>^ it does<br><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Supported validation of settings.<br></blockquote><div>Validation of settings happens in the serializer. In terms of the download API, if the settings are invalid they will be rejected and errors logged so I think everything will work as expected without this requirement.<br><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
- Shared Contexts - collaborative sharing of resources between downloaders.<br>
Supported shared: sessions (connection pools), auth tokens and resolved mirror lists.<br></blockquote><div>It has a shared context. The mirror lists don't exist in either code base but they can easily be added here.<br> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Supported the sharing of resources without requiring the user of the downloader to know<br>
which resources are shared or how they are shared. Resource sharing needs to happen in the<br>
streamer too.<br></blockquote><div>I don't understand this new requirement can you elaborate?<br></div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
- Support for HEAD requests (HttpDownload).<br>
Just fetches the HTTP headers.<br></blockquote><div>We can do this even thought this is a new requirement. Why is this a requirement?<br> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
- Standard exceptions.<br>
Supported catching DownloadError, NotFound and NotAuthorized.<br>
Encapsulated client lib specific exceptions and error behaviors.<br></blockquote><div>^ What are the desired behaviors here exactly? I think launching with fewer behaviors is better for an alpha. These requirements are not written.<br><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
- Downloader Attachments - attach any user defined object to the downloader.<br>
Supported easy correlation between a downloader and an artifact being downloaded.<br></blockquote><div>^ is supported using the 'url' of the DownloadReport or the 'id' of the GroupDownloader<br><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<span class="gmail-"><br>
<br>
> * ^ is surprising considering the majority of lines added are docs<br>
> * Read the compiled docs here [1]<br>
><br>
> # Features<br>
> * It has 100% feature parity with the existing http/https downloaders.<br>
<br>
</span>See gaps above.<br>
<br>
Are authenticated proxies supported?<br></blockquote><div>I think it is supported from this ticket. <a href="https://github.com/aio-libs/aiohttp/issues/845#issuecomment-278727807">https://github.com/aio-libs/aiohttp/issues/845#issuecomment-278727807</a> Even if it doesn't though, it will at some point. aiohttp is a very active project that works closely with the requests community. Pulp is at an alpha and I don't think proxy ssl verification is anything we've talked about as a hard requirement for the 3.0 release.<br> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<span class="gmail-"><br>
Ftp can be added, but purposely left<br>
> out for now. The requirements being fulfilled are written here [2].<br>
> * It has more features than the existing downloaders including additional ssl config, proxy_auth, additional<br>
> connection options, and more.<br>
<br>
</span>Can you enumerate them?<br></blockquote><div>* You can add authentication credentials when authenticating with a proxy. <br></div><div>* You can configure security settings such as what SSL TLS versions are allowed now.<br></div><div>* You can have finer control over flow rate timeouts and other connection options<br></div><div>* you can provide fingerprint verification of ssl certificates<br></div><div>* you can use non-system dns easily through a simple configuration. This is another security feature<br></div><div>* enable/disable DNS caching<br></div><div>There are even more actually ^<br></div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<span class="gmail-"><br>
><br>
> # Easier for plugin writers<br>
> * An easier download customization experience by having to subclass fewer objects than the existing<br>
> downloaders.<br>
<br>
</span>Can you be specific?<br></blockquote><div>This design has so many fewer objects that the plugin writer probably only has to subclass one thing, HttpDownloader. It's less for them to think about and have to customize.<br><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<span class="gmail-"><br>
> This design has one downloader for both synchronous or asynchronous downloading<br>
<br>
</span>But they need to execute the downloader in an Asynio loop for both synchronous and asynchronous, correct?<br></blockquote><div>Correct. They only have one thing to use and one way to do it. Also since it's 3 lines of code even a single download is easily handled.<br><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<span class="gmail-"><br>
> * Makes some functionality contained only in the changeset available in the downloaders directly. Specifically<br>
> the GroupDownloader was functionality you could only get via changeset usage. Changesets could easily be<br>
> retooled to use this instead which would be great.<br>
> * Each object can be used independently or in conjunction with others<br>
><br>
> # Demo<br>
> * works with the HEAD of master of pulp_example [3] (not pulp_file) which shows that it actually runs<br>
><br>
> [0]: <a href="https://github.com/pulp/pulp/pull/3129" rel="noreferrer" target="_blank">https://github.com/pulp/pulp/<wbr>pull/3129</a><br>
> [1]: <a href="http://file.rdu.redhat.com/%7Ebbouters/plugins/plugin-api/download.html" rel="noreferrer" target="_blank">http://file.rdu.redhat.com/~<wbr>bbouters/plugins/plugin-api/<wbr>download.html</a><br>
> [2]: <a href="https://pulp.plan.io/issues/2951#note-17" rel="noreferrer" target="_blank">https://pulp.plan.io/issues/<wbr>2951#note-17</a><br>
> [3]: <a href="https://github.com/dkliban/pulp_example/" rel="noreferrer" target="_blank">https://github.com/dkliban/<wbr>pulp_example/</a><br>
><br>
> -Brian<br>
><br>
><br>
</span>> ______________________________<wbr>_________________<br>
> Pulp-dev mailing list<br>
> <a href="mailto:Pulp-dev@redhat.com">Pulp-dev@redhat.com</a><br>
> <a href="https://www.redhat.com/mailman/listinfo/pulp-dev" rel="noreferrer" target="_blank">https://www.redhat.com/<wbr>mailman/listinfo/pulp-dev</a><br>
><br>
<br>
<br>______________________________<wbr>_________________<br>
Pulp-dev mailing list<br>
<a href="mailto:Pulp-dev@redhat.com">Pulp-dev@redhat.com</a><br>
<a href="https://www.redhat.com/mailman/listinfo/pulp-dev" rel="noreferrer" target="_blank">https://www.redhat.com/<wbr>mailman/listinfo/pulp-dev</a><br>
<br></blockquote></div><br></div></div>