Skip to content

HDDS-7402. Adapt CommandQueue to track the count of each queued command type#3891

Merged
umamaheswararao merged 4 commits intoapache:masterfrom
sodonnel:hdds-7402
Oct 27, 2022
Merged

HDDS-7402. Adapt CommandQueue to track the count of each queued command type#3891
umamaheswararao merged 4 commits intoapache:masterfrom
sodonnel:hdds-7402

Conversation

@sodonnel
Copy link
Contributor

What changes were proposed in this pull request?

Currently, all commands queued for a datanode are stored in an ArrayList, which is sent to the DN on each heartbeat.

In order to limit the number of commands queued on a DN at any time, it would be useful to be able to query the current queued count of each command type, allowing Replication Manager, for example, to stop sending more work to the DN until it has free capacity.

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-7402

How was this patch tested?

New unit tests

Copy link
Contributor

@swamirishi swamirishi left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@swamirishi swamirishi left a comment

Choose a reason for hiding this comment

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

Left a comment, for making the code more modular

// This list is used as default return value.
private static final List<SCMCommand> DEFAULT_LIST = new ArrayList<>();
private final Map<UUID, Commands> commandMap;
private final Map<UUID, Map<SCMCommandProto.Type, Integer>> summaryMap;
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be better if we add another class to handle Commands for one Datanode separately? Like something like DatanodeQueue. So summary and commands can go together in one place.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess that is something we could do in the future if there were more things to track on a datanode by datanode basis. I'd like to avoid too much change here, and a class per DN like a DatanodeQueue would look fairly similar I think. Something like this could easily be added later if we need it, as it will be behind the CommandQueue interface, and the changes would just be internal to the class.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe the summary could go into the commands class, since we already have that

Copy link
Contributor

Choose a reason for hiding this comment

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

I see we are doing the same map operation twice here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea putting the summary into the commands class might make it tidier. I will try that tomorrow.

Copy link
Contributor

@umamaheswararao umamaheswararao left a comment

Choose a reason for hiding this comment

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

LGTM

@umamaheswararao umamaheswararao merged commit 557d7f8 into apache:master Oct 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

Comments