rippled icon indicating copy to clipboard operation
rippled copied to clipboard

Move JobQueue and related classes into xrpl.core module

Open pratikmankawde opened this issue 1 month ago • 1 comments

High Level Overview of Change

JobQueue and related classes are now in xrpl.core module More details in Jira ticket.

Context of Change

This is being done under an initiative of modularizing rippled library.

Type of Change

  • [ ] Bug fix (non-breaking change which fixes an issue)
  • [ ] New feature (non-breaking change which adds functionality)
  • [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • [x] Refactor (non-breaking change that only restructures code)
  • [ ] Performance (increase or change in throughput and/or latency)
  • [ ] Tests (you added tests for code that already exists, or your new feature included in this PR)
  • [ ] Documentation update
  • [ ] Chore (no impact to binary, e.g. .gitignore, formatting, dropping support for older tooling)
  • [ ] Release

Test Plan

Tested with unit tests.

pratikmankawde avatar Dec 08 '25 18:12 pratikmankawde

Codecov Report

:white_check_mark: All modified and coverable lines are covered by tests. :white_check_mark: Project coverage is 79.1%. Comparing base (9eb84a5) to head (21b4516). :warning: Report is 1 commits behind head on develop.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##           develop   #6121   +/-   ##
=======================================
  Coverage     79.0%   79.1%           
=======================================
  Files          839     839           
  Lines        71376   71391   +15     
  Branches      8361    8342   -19     
=======================================
+ Hits         56407   56446   +39     
+ Misses       14969   14945   -24     
Files with missing lines Coverage Δ
include/xrpl/core/ClosureCounter.h 100.0% <ø> (ø)
include/xrpl/core/Coro.ipp 100.0% <ø> (ø)
include/xrpl/core/JobQueue.h 92.3% <ø> (ø)
include/xrpl/core/JobTypeData.h 100.0% <ø> (ø)
include/xrpl/core/JobTypeInfo.h 100.0% <ø> (ø)
include/xrpl/core/JobTypes.h 98.8% <ø> (ø)
include/xrpl/core/PerfLog.h 69.2% <ø> (ø)
include/xrpl/core/detail/Workers.h 100.0% <ø> (ø)
include/xrpl/core/detail/semaphore.h 100.0% <ø> (ø)
src/libxrpl/core/detail/Job.cpp 56.8% <ø> (ø)
... and 34 more

... and 4 files with indirect coverage changes

Impacted file tree graph

:rocket: New features to boost your workflow:
  • :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

codecov[bot] avatar Dec 08 '25 18:12 codecov[bot]

The PerfLog was in its own "module" before within xrpld, but now it got moved to the core module. Does it fit best in the core module, or should it be kept in its own module in libxrpl?

Thinking ahead to the logging refactoring/improving that @a1q123456 is going to do, rather than putting PerfLog in its own module, perhaps it should go into a more general "logging" module?

I'll wait to hear Jingchen's thoughts on this.

pratikmankawde avatar Dec 10 '25 18:12 pratikmankawde

The PerfLog was in its own "module" before within xrpld, but now it got moved to the core module. Does it fit best in the core module, or should it be kept in its own module in libxrpl? Thinking ahead to the logging refactoring/improving that @a1q123456 is going to do, rather than putting PerfLog in its own module, perhaps it should go into a more general "logging" module?

I'll wait to hear Jingchen's thoughts on this.

Yeah, what I’m thinking is we kind of don’t want to have a module with only one class in it and we also don’t want to have a module that contains lots of unrelated stuff.

After some investigation, seems that perflog is relatively small and I think it’s probably not really worth having a standalone module for it.

I think we can probably just put it here and then we can move it out if it becomes a problem.

a1q123456 avatar Dec 10 '25 18:12 a1q123456

The PerfLog was in its own "module" before within xrpld, but now it got moved to the core module. Does it fit best in the core module, or should it be kept in its own module in libxrpl? Thinking ahead to the logging refactoring/improving that @a1q123456 is going to do, rather than putting PerfLog in its own module, perhaps it should go into a more general "logging" module?

I'll wait to hear Jingchen's thoughts on this.

Yeah, what I’m thinking is we kind of don’t want to have a module with only one class in it and we also don’t want to have a module that contains lots of unrelated stuff.

After some investigation, seems that perflog is relatively small and I think it’s probably not really worth having a standalone module for it.

I think we can probably just put it here and then we can move it out if it becomes a problem.

Do you have plans to put logging into separate module? We could also have a module for performance metrics related code. Or shall we add all such code in instrumentation category module?

pratikmankawde avatar Dec 10 '25 19:12 pratikmankawde

The PerfLog was in its own "module" before within xrpld, but now it got moved to the core module. Does it fit best in the core module, or should it be kept in its own module in libxrpl? Thinking ahead to the logging refactoring/improving that @a1q123456 is going to do, rather than putting PerfLog in its own module, perhaps it should go into a more general "logging" module?

I'll wait to hear Jingchen's thoughts on this.

Yeah, what I’m thinking is we kind of don’t want to have a module with only one class in it and we also don’t want to have a module that contains lots of unrelated stuff. After some investigation, seems that perflog is relatively small and I think it’s probably not really worth having a standalone module for it. I think we can probably just put it here and then we can move it out if it becomes a problem.

Do you have plans to put logging into separate module? We could also have a module for performance metrics related code. Or shall we add all such code in instrumentation category module?

The logging infrastructure is currently in xrpl/beast/utility

I’m not sure how many files we have for performance metrics related code but I don’t think there’s a lot

a1q123456 avatar Dec 10 '25 19:12 a1q123456

The PerfLog was in its own "module" before within xrpld, but now it got moved to the core module. Does it fit best in the core module, or should it be kept in its own module in libxrpl? Thinking ahead to the logging refactoring/improving that @a1q123456 is going to do, rather than putting PerfLog in its own module, perhaps it should go into a more general "logging" module?

I'll wait to hear Jingchen's thoughts on this.

Yeah, what I’m thinking is we kind of don’t want to have a module with only one class in it and we also don’t want to have a module that contains lots of unrelated stuff. After some investigation, seems that perflog is relatively small and I think it’s probably not really worth having a standalone module for it. I think we can probably just put it here and then we can move it out if it becomes a problem.

Do you have plans to put logging into separate module? We could also have a module for performance metrics related code. Or shall we add all such code in instrumentation category module?

The logging infrastructure is currently in xrpl/beast/utility

I’m not sure how many files we have for performance metrics related code but I don’t think there’s a lot

Let's revisit this later then if we identify a need during further modularization. Let's keep the PerfLog under core then.

bthomee avatar Dec 10 '25 21:12 bthomee