Skip to content

feat(serializer) : make buffer max capacity configurable#3049

Open
LegendPei wants to merge 4 commits into
apache:masterfrom
LegendPei:feat/serializer-buffer-max-capacity
Open

feat(serializer) : make buffer max capacity configurable#3049
LegendPei wants to merge 4 commits into
apache:masterfrom
LegendPei:feat/serializer-buffer-max-capacity

Conversation

@LegendPei

@LegendPei LegendPei commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

Purpose of the PR

This PR makes the max capacity of one serializer buffer configurable instead of always using the hardcoded default 128MB limit.

The default behavior remains unchanged, while users who need to serialize larger objects can tune the buffer limit through configuration. The new option is still bounded to avoid unbounded memory allocation.

Main Changes

  • Trivial rework / code cleanup without any test coverage. (No Need)
  • Already covered by existing tests, such as (please modify tests here).
  • Need tests and can be verified as follows:
    • xxx

Does this PR potentially affect the following parts?

Documentation Status

  • Doc - TODO
  • Doc - Done
  • Doc - No Need

@dosubot dosubot Bot added size:L This PR changes 100-499 lines, ignoring generated files. feature New feature labels Jun 4, 2026
@codecov

codecov Bot commented Jun 4, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.25%. Comparing base (7f0a44a) to head (a08bdd6).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files
@@              Coverage Diff              @@
##             master    #3049       +/-   ##
=============================================
+ Coverage     36.08%   93.25%   +57.17%     
+ Complexity      338       65      -273     
=============================================
  Files           803        9      -794     
  Lines         68227      267    -67960     
  Branches       8963       22     -8941     
=============================================
- Hits          24617      249    -24368     
+ Misses        40963        8    -40955     
+ Partials       2647       10     -2637     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

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

@imbajin imbajin requested a review from VGalaxies June 5, 2026 03:04

@imbajin imbajin left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The implementation is close and the process-wide scope is the right fit for BytesBuffer. Please avoid last-opened-graph-wins behavior by making that process-wide semantic explicit and rejecting conflicting graph configs.

bitflicker64 added a commit to bitflicker64/incubator-hugegraph that referenced this pull request Jun 7, 2026
tail --pid does not forward signals to the supervised process. Add a
TERM/INT trap that sends SIGTERM to Java and polls with kill -0 until
Java exits cleanly before the entrypoint exits.

Addresses review feedback from imbajin on apache#3049.

@VGalaxies VGalaxies left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Review summary

  • Blocking: yes
  • Summary: The PR leaves graph lock groups registered when the new serializer buffer capacity initialization fails, which can block a corrected retry in the same process.
  • Evidence:
    • Static review
    • git diff --check origin/master...HEAD passed. mvn test -pl hugegraph-server/hugegraph-test -am -P unit-test -Dtest=BytesBufferTest failed before reaching the target tests because hugegraph-pd ran surefire with no tests

…oups

Move BytesBuffer.initMaxBufferCapacity() and other process-wide static
config initializations before LockUtil.init() so that validation failures
(e.g. invalid or conflicting serializer.buffer_max_capacity) cannot leave
orphaned lock groups in LockManager, which would block subsequent graph
load attempts without a process restart.

🤖 Generated with [Qoder][https://qoder.com]

@imbajin imbajin left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Blocking: yes. Summary: The process-wide serializer buffer cap is still sourced from each graph config, so mixed custom/default graph configs can fail multi-graph startup. Evidence: static review of GraphManager local graph loading plus BytesBufferTest passed locally.


// Init process-wide static configs before lock, so that validation
// failures won't leave stale lock groups in LockManager.
BytesBuffer.initMaxBufferCapacity(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

‼️ Keep the process-wide cap from conflicting with default graph configs

Evidence: GraphManager loads every file under conf/graphs in one JVM, and each StandardHugeGraph calls BytesBuffer.initMaxBufferCapacity(config.get(...)). With this option stored as a graph option, the first graph that sets serializer.buffer_max_capacity=268435456 freezes the static cap, then any later graph that omits the option still passes the default 134217728 and fails with the new conflicting-value check.

Impact: enabling this option for one graph can prevent a normal multi-graph server from starting unrelated graphs, even though a process-wide setting should be shared by the process.

Requested fix: move the source to server-wide config, or treat an unset/default graph value as inheriting the already-initialized process value, and add a GraphManager/MultiGraphsTest case covering one custom graph plus one default graph.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature New feature size:L This PR changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Maybe the max buffer capacity should be customized

3 participants