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.

Sign up for GitHub

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

Jump to bottom

HPCC-33279 Serve CKeyBuilder stats through getStatistic #19442

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

HPCC-33279 Serve CKeyBuilder stats through getStatistic #19442

wants to merge 2 commits into from
+41 −74

Conversation

shamser
Copy link
Contributor

@shamser shamser commented Jan 22, 2025 •
edited
Loading

Type of change:

  • This change is a bug fix (non-breaking change which fixes an issue).
  • This change is a new feature (non-breaking change which adds functionality).
  • This change improves the code (refactor or other change that does not change the functionality)
  • This change fixes warnings (the fix does not alter the functionality or the generated code)
  • This change is a breaking change (fix or feature that will cause existing behavior to change).
  • This change alters the query API (existing queries will have to be recompiled)

Checklist:

  • My code follows the code style of this project.
    • My code does not create any new warnings from compiler, build system, or lint.
  • The commit message is properly formatted and free of typos.
    • The commit message title makes sense in a changelog, by itself.
    • The commit is signed.
  • My change requires a change to the documentation.
    • I have updated the documentation accordingly, or...
    • I have created a JIRA ticket to update the documentation.
    • Any new interfaces or exported functions are appropriately commented.
  • I have read the CONTRIBUTORS document.
  • The change has been fully tested:
    • I have added tests to cover my changes.
    • All new and existing tests passed.
    • I have checked that this change does not introduce memory leaks.
    • I have used Valgrind or similar tools to check for potential issues.
  • I have given due consideration to all of the following potential concerns:
    • Scalability
    • Performance
    • Security
    • Thread-safety
    • Cloud-compatibility
    • Premature optimization
    • Existing deployed queries will not be broken
    • This change fixes the problem, not just the symptom
    • The target branch of this pull request is appropriate for such a change.
  • There are no similar instances of the same problem that should be addressed
    • I have addressed them here
    • I have raised JIRA issues to address them separately
  • This is a user interface / front-end modification
    • I have tested my changes in multiple modern browsers
    • The component(s) render as expected

Smoketest:

  • Send notifications about my Pull Request position in Smoketest queue.
  • Test my draft Pull Request.

Testing:

@github-actions GitHub Actions
Copy link

Jira Issue: https://hpccsystems.atlassian.net//browse/HPCC-33279

Jirabot Action Result:
Workflow Transition To: Merge Pending
Updated PR

@shamser shamser changed the title HPCC-33279 Serve stats in CKeyBuilder through getStatistic HPCC-33279 Serve CKeyBuilder stats through getStatistic Jan 22, 2025
@shamser shamser force-pushed the issue33279 branch 2 times, most recently from 2cc1f12 to 1ca7faf Compare January 23, 2025 16:58
@shamser shamser changed the base branch from candidate-9.10.x to master January 23, 2025 16:59
@shamser shamser marked this pull request as ready for review January 23, 2025 16:59
@shamser shamser requested a review from jakesmith January 23, 2025 16:59
@jakesmith jakesmith closed this Jan 28, 2025
@jakesmith jakesmith reopened this Jan 28, 2025
@github-actions GitHub Actions
Copy link

Jira Issue: https://hpccsystems.atlassian.net//browse/HPCC-33279

Jirabot Action Result:
Workflow Transition To: Merge Pending

@shamser
HPCC-33279 Serve CKeyBuilder stats through getStatistic
9ec93c5 
- Serve CKeyBuilder statistics through getStatistic - the standard method of serving statistics.
- Publish 4 new statistics for index write activity: StNumDuplicateKeyCount, StNumOffsetBranches,
StSizeBranchMemory and StSizeLeafMemory.

Signed-off-by: Shamser Ahmed <[email protected]>
jakesmith
jakesmith reviewed Jan 29, 2025
View reviewed changes
Copy link
Member

@jakesmith jakesmith left a comment

Choose a reason for hiding this comment

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

@shamser - 1 minor issue.

system/jlib/jstats.cpp Outdated
@@ -999,6 +999,11 @@ static const constexpr StatisticMeta statsMetaData[StMax] = {
{ PEAKNUMSTAT(PeakCacheObjects), "High water mark for number of objects in a cache"},
{ NUMSTAT(CacheDuplicates), "The number of times an item was added to a cache by two threads at the same time" },
{ NUMSTAT(CacheEvictions), "The number of times an item was evicted from a cache" },
{ NUMSTAT(DuplicateKeyCount), "The number of duplicate keys" },
{ NUMSTAT(OffsetBranches), "The number of offset branches" },
Copy link
Member

Choose a reason for hiding this comment

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

This is not a number of offset branches - it is the [single] offset into the index where the branch nodes starts (vs the leaf nodes). So I think this should be SIZESTAT and description something like "The 1st branch node offset position in the index"

jakesmith
jakesmith reviewed Jan 30, 2025
View reviewed changes
thorlcr/activities/indexwrite/thindexwriteslave.cpp Outdated
mb.append(branchMemorySize);
mb.append(leafMemorySize);
mb.append(inactiveStats.getStatisticValue(StSizeBranchMemory));
mb.append(inactiveStats.getStatisticValue(StSizeLeafMemory));
Copy link
Member

Choose a reason for hiding this comment

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

these stats are now already being serialized to the manager (via regular stats. serialization),
so I think these lines should be deleted, and the corresponding manual deserialization in the manager activity code, and the manager activity should pick up these values from CMasterActivity::statsCollection..

e.g. in IndexWriteActivityMaster::done:

            props.setPropInt64("@numLeafNodes", statsCollection.getStatisticSum(StNumLeafCacheAdds));

Copy link
Member

Choose a reason for hiding this comment

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

not urgent, so can be left to a subsequent JIRA/PR if you prefer?
Should be a relatively easy change and avoid future confusion re. 2 places these stats. are serialized/deserialized.

@shamser shamser requested a review from jakesmith January 31, 2025 11:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jakesmith jakesmith Awaiting requested review from jakesmith

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@shamser @jakesmith