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

Improve names returned by server_info counters #4031

Closed
wants to merge 2 commits into from

Conversation

natenichols
Copy link
Contributor

@natenichols natenichols commented Dec 14, 2021

@scottschurr authored this commit and it has been running on reporting servers.

High Level Overview of Change

Historically, quite a number of different kinds of operations have been running in the JobQueue as jtCLIENT. These include:

  • fee change reporting,
  • consensus state change reporting,
  • returning account history,
  • websocket subscriptions,
  • client requested shard archiving,
  • RPC requests, and
  • websocket requests.

Placing all of these operation types under jtCLIENT made them indistinguishable when server_info returned usage of the JobQueue. This commit improves visibility of the operations being performed in the JobQueue by providing unique jtClient* priorities listed above.

This change has the side effect of giving the different operations different priorities in the queue. I don't think this will have any negative impact on the ledger. And none has been observed in the reporting mode servers. But it's worth thinking about during the review. The priorities of the new job classifications are, from low to high priority:

    jtCLIENT_SUBSCRIBE,   // A websocket subscription by a client
    jtCLIENT_FEE_CHANGE,  // Subscription for fee change by a client
    jtCLIENT_CONSENSUS,   // Subscription for consensus state change by a client
    jtCLIENT_ACCT_HIST,   // Subscription for account history by a client
    jtCLIENT_SHARD,       // Client request for shard archiving
    jtCLIENT_RPC,         // Client RPC request
    jtCLIENT_WEBSOCKET,   // Client websocket request

These priorities can be trivially changed (by re-ordering their appearance in the enum) if any reviewer sees a good reason to do so.

Context of Change

When debugging overloading of the JobQueue it was noted that many of the entries simply said clientCommand. We wanted to understand which jobs were actually running in the JobQueue. Separating out the various jobs being handled under the clientCommand umbrella was an easy way to accomplish the goal without adding to the pre-existing complexity.

Type of Change

  • Bug fix (non-breaking change which fixes an issue)

No unit tests were added for this change.

@scottschurr
Copy link
Collaborator

Hi @natenichols, I wrote this code, so I don't feel like I can impartially review it. Sorry. But I'd be happy to have you review it.

@natenichols
Copy link
Contributor Author

natenichols commented Dec 14, 2021

Hi @natenichols, I wrote this code, so I don't feel like I can impartially review it. Sorry. But I'd be happy to have you review it.

I can't approve my own pull requests.

@natenichols natenichols removed the request for review from scottschurr December 14, 2021 22:17
add(jtLEDGER_REQ, "ledgerRequest", 5, 0ms, 0ms);
add(jtPROPOSAL_ut, "untrustedProposal", maxLimit, 500ms, 1250ms);
add(jtREPLAY_TASK, "ledgerReplayTask", maxLimit, 0ms, 0ms);
add(jtLEDGER_DATA, "ledgerData", 5, 0ms, 0ms);
Copy link
Contributor

Choose a reason for hiding this comment

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

The limit for this job type used to be 4. Now it's 5. Was the change intentional?

add(jtVALIDATION_ut, "untrustedValidation", maxLimit, 2000ms, 5000ms);
add(jtTRANSACTION_l, "localTransaction", maxLimit, 100ms, 500ms);
add(jtREPLAY_REQ, "ledgerReplayRequest", 10, 250ms, 1000ms);
add(jtLEDGER_REQ, "ledgerRequest", 5, 0ms, 0ms);
Copy link
Contributor

Choose a reason for hiding this comment

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

The limit for this job type used to be 4. Now it's 5. Was this intentional?

Copy link
Collaborator

Choose a reason for hiding this comment

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

No. That was not an intentional change. Good catch. Thanks.

@natenichols natenichols added the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label Dec 15, 2021
This was referenced Dec 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants