Paths

Table of Contentst

Differential D8448

Reorganize configuration of the http server building
ClosedPublic
Actions

Authored by douardda on Sep 12 2022, 3:07 PM.

Details

Reviewers

olasd

Group Reviewers

Reviewers

Commits

rDGRPHf294538188a8: Reorganize configuration of the http server building

Summary

rework the configuration for swh.graph.http_server.make_app() so that
one can start the http server usig an existing grpc server by a setting
the configuration file accordingly.

This make the make_app() function only accept one config structure
(dict), which structure would be:

graph:
  cls: local
  grpc_server: # config for spawn_java_grpc_server()
    ...
  rpc_server: # config for GraphServerApp()
    ...

or:

graph:
  cls: remote
  url: # host:port of the running grpc server
    ...
  rpc_server: # config for GraphServerApp()
    ...

This also comes wit a few other refactorings:

rename swh/graph/rpc/ as swh/graph/grpc/ to prevent confusion with the (http) rpc server.
similarly, rename swh/graph/rpc_server.py as swh/graph/grpc_server.py
remove the "graph" config section from the grpc_server config structure (it had only the "path" config entry).

Depends on D8353

Diff Detail

Repository

rDGRPH Compressed graph representation

Lint

Automatic diff as part of commit; lint not applicable.

Unit

Automatic diff as part of commit; unit tests not applicable.

Event Timeline

douardda created this revision.Sep 12 2022, 3:07 PM

Herald added a reviewer: Reviewers. · View Herald TranscriptSep 12 2022, 3:07 PM

Build was aborted

Patch application report for D8448 (id=30457)

Could not rebase; Attempt merge onto 071271daa4...

Updating 071271d..b33f8d0
Fast-forward
 Makefile.local                                    |   2 +-
 conftest.py                                       |   8 +
 mypy.ini                                          |   2 +-
 pyproject.toml                                    |   2 +-
 setup.cfg                                         |   2 +-
 swh/graph/{rpc => grpc}/swhgraph.proto            |   0
 swh/graph/grpc/swhgraph_pb2.py                    | 196 +++++++++
 swh/graph/{rpc => grpc}/swhgraph_pb2.pyi          | 472 +++++++++++-----------
 swh/graph/{rpc => grpc}/swhgraph_pb2_grpc.py      |  86 ++--
 swh/graph/{rpc_server.py => grpc_server.py}       |  15 +-
 swh/graph/http_server.py                          |  68 +++-
 swh/graph/{tests/conftest.py => pytest_plugin.py} |  19 +-
 swh/graph/rpc/swhgraph_pb2.py                     | 196 ---------
 swh/graph/tests/test_grpc.py                      |   2 +-
 14 files changed, 569 insertions(+), 501 deletions(-)
 create mode 100644 conftest.py
 rename swh/graph/{rpc => grpc}/swhgraph.proto (100%)
 create mode 100644 swh/graph/grpc/swhgraph_pb2.py
 rename swh/graph/{rpc => grpc}/swhgraph_pb2.pyi (60%)
 rename swh/graph/{rpc => grpc}/swhgraph_pb2_grpc.py (70%)
 rename swh/graph/{rpc_server.py => grpc_server.py} (73%)
 rename swh/graph/{tests/conftest.py => pytest_plugin.py} (86%)
 delete mode 100644 swh/graph/rpc/swhgraph_pb2.py

Changes applied before test

commit b33f8d0676d8b4e3ac35c31614a30abbfae30fc8
Author: David Douard <david.douard@sdfa3.org>
Date:   Mon Sep 12 14:48:47 2022 +0200

    Reorganize configuration of the http server building
    
    rework the configuration for swh.graph.http_server.make_app() so that
    one can start the http server usig an existing grpc server by a setting
    the configuration file accordingly.
    
    This make the `make_app()` function only accept one config structure
    (dict), which structure would be:
    
      graph:
        cls: local
        grpc_server: # config for spawn_java_grpc_server()
          ...
        rpc_server: # config for GraphServerApp()
          ...
    
    or:
    
      graph:
        cls: remote
        url: # host:port of the running grpc server
          ...
        rpc_server: # config for GraphServerApp()
          ...
    
    This also comes wit a few other refactorings:
    
    - rename swh/graph/rpc/ as swh/graph/grpc/ to prevent confusion with the
      (http) rpc server.
    - similarly, rename swh/graph/rpc_server.py as swh/graph/grpc_server.py
    - remove the "graph" config section from the grpc_server config
      structure (it had only the "path" config entry).

commit 7049b584c24fd2d6627b6ecb7bf750a0ab063d84
Author: David Douard <david.douard@sdfa3.org>
Date:   Wed Aug 31 11:12:00 2022 +0200

    Move swh-graph's test fixture in a pytest_plugin module

Link to build: https://jenkins.softwareheritage.org/job/DGRPH/job/tests-on-diff/232/
See console output for more information: https://jenkins.softwareheritage.org/job/DGRPH/job/tests-on-diff/232/console

Harbormaster returned this revision to the author for changes because remote builds failed.Sep 12 2022, 3:23 PM

Harbormaster failed remote builds in B31467: Diff 30457!

Build is green

Patch application report for D8448 (id=30457)

Rebasing onto 7049b584c2...

Current branch diff-target is up to date.

Changes applied before test

commit b33f8d0676d8b4e3ac35c31614a30abbfae30fc8
Author: David Douard <david.douard@sdfa3.org>
Date:   Mon Sep 12 14:48:47 2022 +0200

    Reorganize configuration of the http server building
    
    rework the configuration for swh.graph.http_server.make_app() so that
    one can start the http server usig an existing grpc server by a setting
    the configuration file accordingly.
    
    This make the `make_app()` function only accept one config structure
    (dict), which structure would be:
    
      graph:
        cls: local
        grpc_server: # config for spawn_java_grpc_server()
          ...
        rpc_server: # config for GraphServerApp()
          ...
    
    or:
    
      graph:
        cls: remote
        url: # host:port of the running grpc server
          ...
        rpc_server: # config for GraphServerApp()
          ...
    
    This also comes wit a few other refactorings:
    
    - rename swh/graph/rpc/ as swh/graph/grpc/ to prevent confusion with the
      (http) rpc server.
    - similarly, rename swh/graph/rpc_server.py as swh/graph/grpc_server.py
    - remove the "graph" config section from the grpc_server config
      structure (it had only the "path" config entry).

See https://jenkins.softwareheritage.org/job/DGRPH/job/tests-on-diff/235/ for more details.

Harbormaster completed remote builds in B31467: Diff 30457.Sep 12 2022, 3:49 PM

douardda requested review of this revision.Sep 12 2022, 3:49 PM

what about http_rpc_server instead of rpc_server (in the config) and http_server (as module name), to remove any ambiguity?

swh/graph/http_rpc_server.py
343
362–366
swh/graph/pytest_plugin.py
56	`sentry_sdk.capture_exception(e)`

In D8448#219434, @vlorentz wrote:

what about http_rpc_server instead of rpc_server (in the config) and http_server (as module name), to remove any ambiguity?

yeah I though about doing this as well, let's do it them

swh/graph/http_rpc_server.py
343	some day I'll remember this...

rename http_server.py as http_rpc_server.py and use the same http_rpc_server config entry name (as suggested by vlorentz)

Build is green

Patch application report for D8448 (id=30466)

Rebasing onto 7049b584c2...

Current branch diff-target is up to date.

Changes applied before test

commit f294538188a847e0b8430123f4c5d46e0ed1e19d
Author: David Douard <david.douard@sdfa3.org>
Date:   Mon Sep 12 14:48:47 2022 +0200

    Reorganize configuration of the http server building
    
    rework the configuration used by `make_app()` so that one can start the
    http server usig an existing grpc server by a setting the configuration
    file accordingly.
    
    This make the `make_app()` function only accept one config structure
    (dict), which structure would be:
    
      graph:
        cls: local
        grpc_server: # config for spawn_java_grpc_server()
          ...
        http_rpc_server: # config for GraphServerApp()
          ...
    
    or:
    
      graph:
        cls: remote
        url: # host:port of the running grpc server
          ...
        http_rpc_server: # config for GraphServerApp()
          ...
    
    This also comes wit a few other refactorings:
    
    - rename swh/graph/rpc/ as swh/graph/grpc/ to prevent confusion with the
      (http) rpc server.
    - similarly, rename swh/graph/rpc_server.py as swh/graph/grpc_server.py
    - and rename swh/graph/http_server.py as swh/graph/http_rpc_server.py
    - remove the "graph" config section from the grpc_server config
      structure (it had only the "path" config entry).

See https://jenkins.softwareheritage.org/job/DGRPH/job/tests-on-diff/236/ for more details.

Harbormaster completed remote builds in B31474: Diff 30466.Sep 12 2022, 4:27 PM

I think this makes sense.

Could we add a CLI endpoint to spawn a standalone java process? (which would do the same as swh.graph.grpc_server.spawn_java_grpc_server, but using execve instead of popen?)

This revision is now accepted and ready to land.Sep 13 2022, 2:02 PM

Closed by commit rDGRPHf294538188a8: Reorganize configuration of the http server building (authored by douardda). · Explain WhySep 13 2022, 3:01 PM

This revision was automatically updated to reflect the committed changes.

douardda added a commit: rDGRPHf294538188a8: Reorganize configuration of the http server building.