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

File transfer priority level configuration requirement clarification #239

Closed
3 tasks done
semaldona opened this issue May 6, 2022 · 11 comments · Fixed by #248
Closed
3 tasks done

File transfer priority level configuration requirement clarification #239

semaldona opened this issue May 6, 2022 · 11 comments · Fixed by #248
Assignees
Milestone

Comments

@semaldona
Copy link

semaldona commented May 6, 2022

Checklist (Please check before submitting)

  • I reviewed the Contributing Guide.
  • I reviewed the CF README.md file to see if the feature is in the major future work.
  • I performed a cursory search to see if the feature request is relevant, not redundant, nor in conflict with other tickets.

Is your feature request related to a problem? Please describe.
No

Describe the solution you'd like
Per requirement CF5030.1, the CF file-transfer priority levels shall be configurable. This functionality is not currently implemented in CF.

Describe alternatives you've considered
None

Additional context
None

Requester Info
Sergio Maldonado NASA/GSFC/Arctic Slope Technical Services

@skliper
Copy link
Contributor

skliper commented May 6, 2022

Isn't this requirement just saying that the priority level for a file transfer can configured by the user... vs saying that the priority levels themselves (0-255 or whatever) can be changed to something else (like 0-10)? If it's the former, then it's right here:

uint8 priority; /**< \brief priority to use when placing transactions on the pending queue */

CF/fsw/src/cf_utils.c

Lines 323 to 339 in d84a3ee

int CF_PrioSearch(CF_CListNode_t *node, void *context)
{
CF_Transaction_t * t = container_of(node, CF_Transaction_t, cl_node);
CF_Traverse_PriorityArg_t *p = (CF_Traverse_PriorityArg_t *)context;
if (t->priority <= p->priority)
{
/* found it!
*
* the current transaction's prio is less than desired (higher)
*/
p->t = t;
return CF_CLIST_EXIT;
}
return CF_CLIST_CONT;
}

@semaldona
Copy link
Author

I think it's the latter.

CF5030: Each CF output channel shall have 256 file-transfer priority levels.
CF5030.1: The CF file-transfer priority levels shall be configurable
CF5030.2: The highest file-transfer priority level shall be zero.

Reading 5030.1 along with the others, it seems as though the required functionality is to limit the range of priorities. Plus, there are other requirements that already call out using the priority in a command argument.

@skliper
Copy link
Contributor

skliper commented May 6, 2022

Let's get input from @astrogeco and @jphickey. I don't think there's any need to have a requirement to be able to change the number of priority levels by configuration (this really doesn't seem to add much in terms of useful functionality that I can tell). I do think the priority level for a file transfer shall be configurable. Maybe the requirement just needs to be rewritten?

@skliper
Copy link
Contributor

skliper commented May 6, 2022

Note that if you can change the number of priority levels by configuration, then that configuration would fail CF5030. Could be as simple as removing an "s" in CF5030.1: The CF file-transfer priority level shall be configurable.

@skliper
Copy link
Contributor

skliper commented May 6, 2022

Although it seems implied by the .1 relationship, might be better to say:
CF5030.1: The CF output channel file-transfer priority level shall be configurable.

I'm still guessing on intent here so we should get input from others... but that's how I understand it. It's explicitly requiring the priority as an element in the table for each output channel.

@semaldona
Copy link
Author

Agreed on both your interpretation and that a rewrite is in order. However it doesn't seem that "configurable" is the appropriate term either. In the context of most other CF requirements, configuration items and actions typically denote behavior that is table driven. File transfer command arguments, such as class, channel, and priority level seem to capture the run-time behavior.

I would suggest a modification similar to CF5030.1: The CF output channel file-transfer priority level shall be set by a CF command.

While this is somewhat redundant with an existing requirement, CF3000: When CF receives a "Transfer File" command, CF shall play back the file indicated by the command-specified: filename, source path, destination path, keep/delete flag, service class, priority, channel, and peer-entity id, it could be left in for clarity.

@skliper
Copy link
Contributor

skliper commented May 6, 2022

Maybe we should chat :) It's in the table per my link above... so doesn't that mean table driven hence, "configurable"?

@skliper
Copy link
Contributor

skliper commented May 6, 2022

Basically, for the transfers that aren't commanded (polling or whatever) it uses the priority as specified in the config table doesn't it?

@semaldona
Copy link
Author

Yes you are correct, priority is both a table item when polling and a parameter for commanded transfers.

@skliper
Copy link
Contributor

skliper commented May 6, 2022

Ok cool, so maybe then:
CF5030.1: The CF output channel file-transfer default priority level shall be configurable.
and somewhere in the rational state the default priority is used for transfers that are not initiated by command. Commanded transfers override the configured default priority with the priority contained in the command.

@semaldona
Copy link
Author

Agreed, this sounds like the optimal solution.

@astrogeco astrogeco added this to the Draco milestone May 11, 2022
@skliper skliper changed the title File transfer priority levels should be configurable File transfer priority level configuration requirement clarification May 13, 2022
skliper added a commit to skliper/CF that referenced this issue May 24, 2022
skliper added a commit to skliper/CF that referenced this issue May 24, 2022
astrogeco added a commit that referenced this issue May 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants