add keepalive_timeout flag #6674
Conversation
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
|
Please run |
|
@mjjbell Thanks for your comment, I have fixed the format problem, could you rerun the workflow? |
fenwuyaoji
force-pushed
the
keepalive_timeout-flag
branch
from
c3a8e5e
to
31c044a
Compare
|
@mjjbell Sorry for the late response. I checked the page https:/Project-OSRM/osrm-backend/blob/master/CONTRIBUTING.md and added |
|
@mjjbell Sorry to disturb you again. I saw there was one test case that didn't pass. I'm not familiar with the js test so I just copied and modified another one as mine. Now I notice that, if I want to throw an error, I should first define it in node_osrm_support.hpp. I checked the case and thought it unnecessary so I deleted it. Could you rerun it? |
|
@mjjbell Really appreciate! Have a good day~ |
test/nodejs/index.js
Outdated
| @@ -131,6 +131,12 @@ test('constructor: throws if default_radius is not a number', function(assert) { | |||
| assert.ok(new OSRM({algorithm: 'MLD', path: monaco_mld_path, default_radius: 'unlimited'}), 'Does accept unlimited'); | |||
| }); | |||
|
|
|||
| test('constructor: takes a keepalive_timeout argument', function(assert) { | |||
| assert.plan(1); | |||
| var osrm = new OSRM({algorithm: 'MLD', path: monaco_mld_path, keepalive_timeout: 10}); | |||
There was a problem hiding this comment.
The Node bindings do not use the HTTP server, so the keepalive_timeout won't do anything here.
I would just remove the test.
There was a problem hiding this comment.
Sure, I just remove it.
Issue
[What issue is this PR targeting? If there is no issue that addresses the problem, please open a corresponding issue and link it here.]
Relevant to:
#6673 Rquire a configurable parameter to set keepalive_timeout.
Please read our documentation on release and version management.
If your PR is still work in progress please attach the relevant label.
Tasklist
Requirements / Relations
Link any requirements here. Other pull requests this PR is based on?