Move priority check to Job constructor instead of input parser

[krashish8]

I see that the range of task priority is restricted between [0, 100] for the JSON-API user in the input_parser.cpp, but such checks are not provided in the internal C++ code. So, in C++, the range of priority can go high up to the upper limit of uint64.

vroom/src/utils/input_parser.cpp

Lines 112 to 124 in b06900e

	inline Duration get_priority(const rapidjson::Value& object) {
	Priority priority = 0;
	if (object.HasMember("priority")) {
	if (!object["priority"].IsUint()) {
	throw Exception(ERROR::INPUT, "Invalid priority value.");
	}
	priority = object["priority"].GetUint();
	if (priority > MAX_PRIORITY) {
	throw Exception(ERROR::INPUT, "Invalid priority value.");
	}
	}
	return priority;
	}

Shall we consider this as a "feature" instead of a bug, and assume that priority checks won't be added in the future in C++? ;-)

This is actually useful if we want to have some "pre-planned tasks" whose schedule won't change. Obviously, if someone is using libvroom, then custom checks can be added on the client-side for priority before sending data to VROOM.

So, on the client-side, one can keep the priority of such pre-planned tasks "very very high" (such as 10^9, instead of 100 because there is no restriction in libvroom), and let the other normal tasks have priority in the range [0, 100]. I understand that this still won't guarantee that pre-planned tasks are always allocated some schedule, but their probability would be very high. And since no checks for priority are added on the VROOM side, this feature can be exploited to make this happen. :-)

[jcoupey]

Good catch, I just omitted to perform the check on the C++ side. The usual rule is to validate input at the C++ level so that the behavior is consistent whether we use C++ functions or the json API. For example validity for job time windows happens in the job ctor.

I think we'd just have to move the check against MAX_PRIORITY there, any PR welcome. ;-)

On the question of having a "very very high" priority, this is the reason for allowing priorities up to 100 (initially we only had values up to 10).

[krashish8]

Sure, will make the PR in a couple of days.

Thanks for answering this at the right time. I was just about to exploit this hack to give very very high priority of say, 100000 to the tasks. ;-)

For our application, I was thinking of allowing priority in the range [0, 100] for the end-users, and to make sure that any already scheduled tasks are not unallocated later, I would be giving a very high priority, say 100000 to them internally. But, after this check gets added, maybe I'll remove the priority from the users' side (always = 0 priority), and give priority = 100 to the pre-planned tasks.

[jcoupey]

If you only have two levels of priority, including 0, then the actual value does not matter: since we optimize first on sum of priorities, non-zero priorities will always take over regardless of if the actual value is 1 or 100.

The only problem is if you have several non-zero priority levels, then you could see summing effects: e.g. including 101 jobs with priority 1 would take over including a single job with priority 100. Frankly, I've never seen this to be a problem in real-life instances, and if someone were to complain I'd simply suggest to adjust the MAX_PRIORITY value prior to building. ;-)