Repository navigation
Conversation
- Add selectable forward and embedded REST executors with forward as default. - Reuse the existing Gremlin workers, serializer, auth context and transactions. - Integrate executor startup, shutdown and supported-topology validation. - Add differential contract tests, usage docs and reproducible benchmarks.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #3388 +/- ##
============================================
+ Coverage 55.93% 56.56% +0.62%
- Complexity 2277 6782 +4505
============================================
Files 774 779 +5
Lines 68534 72750 +4216
Branches 9333 10736 +1403
============================================
+ Hits 38334 41150 +2816
- Misses 26634 27885 +1251
- Partials 3566 3715 +149
*This pull request uses carry forward flags. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
- Initialize the executor mock used by the isolated server stop fixture. - Assert admission shutdown precedes Gremlin and REST shutdown. - Validate the timeout regression with 48 Java 17 regression tests.
imbajin
left a comment
There was a problem hiding this comment.
Blocking: no. Summary: Embedded REST execution preserves worker-side context, transaction completion and serialization behavior, with explicit configuration guards. Evidence: Exact-head source review, comparison with pinned TinkerPop 3.8.1 behavior, passing server memory/RocksDB and macOS ARM CI; local runtime tests were not rerun. Score: 8.4/10.
- Resolve every configured Gremlin target address before enabling embedded mode. - Reject mixed loopback and non-loopback DNS answers. - Match the selected listener bind independent of target answer order. - Add deterministic IPv4, IPv6, wildcard and mixed-address regression tests.
- Remove the REST execution-mode option and select embedded execution for local listener targets. - Support local interfaces, host aliases and TLS listeners while preserving remote forwarding. - Cover automatic selection, DNS address checks and a live TLS listener with regression tests. - Update configuration docs and the baseline/candidate benchmark runner.
imbajin
left a comment
There was a problem hiding this comment.
Blocking: yes. Summary: Local target detection can route requests to a different Gremlin server than the configured endpoint. Evidence: The address checks accept distinct local bind and target addresses at the same port; existing tests also accept an IPv6 target for an IPv4-only bind.
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: yes. Summary: in-process execution reuses the Gremlin workers correctly, but local target detection accepts endpoints this listener does not own, and unsupported local configurations now fail startup with no way to keep HTTP forwarding.
- Match every target address against the listener bind and handle wildcard address families conservatively. - Keep HTTP forwarding with a warning for unsupported local execution configurations. - Cover distinct same-port listeners, custom Channelizer startup, authentication-file fallback and TLS behavior. - Compare error response semantics independently of Groovy cache-dependent stack frames and document routing rules.
Purpose of the PR
Remove the internal HTTP hop for REST
GET /gremlinandPOST /gremlinwhen their target is the Gremlin Server co-started in the same process. Targets confirmed to belong to the co-started listener automatically reuse the existing executor when their configuration supports embedded execution; other targets and unsupported configurations retain HTTP forwarding. No execution-mode configuration option is required, and TLS listeners support local execution.Builds on the merged Java 17 (#3261) and TinkerPop 3.8.1 (#3263) upgrades. Design and investigation.

Main Changes
Readiness, new overload controls, authentication-cache consistency and global shutdown redesign remain separate tasks.
Verifying these changes
e19099ef3.HStore, full TinkerPop suites and real authentication-store integration remain unverified. Earlier remote RocksDB and HTTP comparisons are diagnostic evidence from earlier revisions; this change has not established production capacity or statistical performance acceptance.
Does this PR potentially affect the following parts?
Documentation Status
Doc - TODO: required documentation is pending; complete it before merging.Doc - Done: documentation is included here or linked below.Doc - No Need: no user-visible documentation is affected.Documentation: REST Gremlin execution, linked from the Server README and shipped configuration.
Embedded execution is selected only for a matching co-started listener and supported configuration. Specific binds require every resolved target address to match exactly; wildcard binds require concrete local addresses in the same address family. Unsupported Channelizer/authorizer/authentication settings or unresolved/mismatched authentication files use HTTP forwarding with a warning. Authentication-file matching applies when configured. Selection happens before execution, with no fallback retry after execution starts. Design illustration, not HStore, authentication-store, performance or RC acceptance.