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

Round-robin over available Download Client #2278

Merged
merged 2 commits into from Jun 8, 2019

Conversation

Taloth
Copy link
Member

@Taloth Taloth commented Nov 8, 2017

Database Migration

YES - 132
Adding Priority column to DownloadClients, defaults to 1.
Migration numbers enabled clients for backward compat preserving the old 'First' logic.

Description

The round-robin is tested only by unit-tests so far, never actually ran it.
But the problem is that once we do this, it'll round-robin instead of picking the first. Which may affect existing setup and does not allow people to have a 'primary' and 'fallback' mechanism till we have a priority setting.
So up for review, but otherwise parked till we can get that fleshed out.

Todos

  • Tests
  • Config option for prioritization instead of round-robin only.

Issues Fixed or Closed by this PR

@Taloth Taloth added this to the v3.x milestone Dec 19, 2017
@devbrian
Copy link
Contributor

devbrian commented May 7, 2018

Just wanted to make a note on this, I have been using this in production on my server for a month now with no issues.

@devbrian
Copy link
Contributor

Any update on this being added to phantom possibly? @markus101

@sachk
Copy link

sachk commented Apr 20, 2019

Current status on this? I'd be nice to see this in master.

@devbrian
Copy link
Contributor

any chance on getting this added to V3?

@fryfrog
Copy link

fryfrog commented May 28, 2019

For this to be feasible, I'd think one would need to be able to toggle between round robin and all.

@Taloth
Copy link
Member Author

Taloth commented May 29, 2019

@fryfrog not really, 'all' has no meaning. Currently Sonarr picks the first, or if the first is down, it'll pick the second.

@devbrian Yeah, I'll look into getting it merged in, although 3.x basically means "3.0 or later".

@fryfrog
Copy link

fryfrog commented May 29, 2019

If you're going to the trouble of adding this "first" or "round-robin" option, you should go to the trouble of also adding an "all" option. I've seen it asked for as much as round robin.

Edit: And by "asked for as much", I've seen both requested maybe twice each. This is a very niche feature. ;)

@Taloth
Copy link
Member Author

Taloth commented May 29, 2019

But how do you envision 'All'? It's not like you want the same torrent to be submitted to all clients simultaneously.

@fryfrog
Copy link

fryfrog commented May 29, 2019

I certainly don't, nor do I even want round robin. But I've literally had multiple requests by people who want to send them to all their download clients. They both seem about as equally useful to me, which is not very.

All could be used by people w/ a seedbox and local torrents where the seedbox is used for seeding and the local is what they use for importing. It could also be used for multiple uncoordinated sites, kind of the opposite of round robin.

They're all dumb uses and honestly I think this will just complicate the UI for 99% of users. But if you're going to all this trouble to provide a super niche option... at least make the other two people happy too.

@Taloth
Copy link
Member Author

Taloth commented May 29, 2019

But that is a huge complication, because suddenly you have to ignore one client when considering imports. They should have a pp-script to sync the remote torrent to local.
The idea behind round-robin/fallbacks is that you can have have a main client that does most of the work, but a fallback client on a NAS that catches the slack. The current UI allows the user the add multiple clients, but literally the sequence at which you add them determines the priority.
It's definitely a niche feature. I'll probably have to dive into my irc chat logs to find out why I even started working on it, but here we are... 5 inches from the finish.

@fryfrog
Copy link

fryfrog commented May 30, 2019 via email

@Taloth Taloth force-pushed the download-client-roundrobin branch from c8695ba to f3435f9 Compare May 30, 2019 09:48
@Taloth Taloth removed the wip label May 30, 2019
@Taloth Taloth changed the base branch from develop to phantom-develop May 30, 2019 09:51
@Taloth
Copy link
Member Author

Taloth commented May 30, 2019

Added Client Priority and changed to phantom-develop.

@markus101 I opted to put it at the bottom because the hint text meant putting it next to Enable didn't make it look any better.

image

In the overview I also added a "Prio: x" badge if Prio > 1. It currently has kind.DISABLED because it looks best, but some tweaking there might be necessary.

image

@devbrian
Copy link
Contributor

devbrian commented Jun 6, 2019

Hopefully we get a merge soon :). Great work Taloth!

@fryfrog
Copy link

fryfrog commented Jun 6, 2019

How about Pri instead of Prio? :)

Edit: Or honestly, you've got Enabled and Disabled, you might as well just do Priority: 1.

@Taloth
Copy link
Member Author

Taloth commented Jun 6, 2019

Pri could be Primary.

it's waiting for a review from @markus101

@fryfrog
Copy link

fryfrog commented Jun 6, 2019

True, so just write the whole word. Don't abbreviate one and not the other. :)

@Taloth
Copy link
Member Author

Taloth commented Jun 6, 2019

I hate you... 😄

Copy link
Member

@markus101 markus101 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haven't tested the full behaviour, but started it up and it's mostly working as expected, just a couple small tweaks then merge it.

kind={kinds.DISABLED}
outline={true}
>
Prio: {priority}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with the previous discussion, might as well be Priority: x

using (var updateCmd = conn.CreateCommand())
{
updateCmd.Transaction = tran;
updateCmd.CommandText = "UPDATE DownloadClients SET Priority = ? WHERE Id = ?";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we be increasing the priority of clients of different types?

I have a torrent client and usenet client enabled, but my torrent client got priority 2 because I added it second. Instead I would expect to see both have a priority of 1.

A simple fix would be to have two counters, one for each protocol.

@Taloth Taloth force-pushed the download-client-roundrobin branch from cce59fa to 1d77c40 Compare June 8, 2019 13:51
@Taloth Taloth merged commit 1d77c40 into phantom-develop Jun 8, 2019
@fryfrog
Copy link

fryfrog commented Jun 8, 2019

:o

@Taloth
Copy link
Member Author

Taloth commented Jun 8, 2019

Less than 2 years. Got even older branches.

@Taloth Taloth deleted the download-client-roundrobin branch June 8, 2019 16:33
@Taloth
Copy link
Member Author

Taloth commented Jun 11, 2019

@devbrian Have you been able to test the latest v3 version? (Lidarr/Radarr wants to know if it's safe to port over 😄)

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 22, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants