Skip to content

Comments

Support configure global level client version string#208

Closed
BewareMyPower wants to merge 1 commit intoapache:mainfrom
BewareMyPower:bewaremypower/client-version
Closed

Support configure global level client version string#208
BewareMyPower wants to merge 1 commit intoapache:mainfrom
BewareMyPower:bewaremypower/client-version

Conversation

@BewareMyPower
Copy link
Contributor

Motivation

Currently C++ client generates a Version.h based on the Version.h.in in the templates directory when compiling. The header includes two macros:

  • PULSAR_VERSION: Never used
  • PULSAR_VERSION_STR: Used as the client_version field in CommandConnect

Once the library (e.g. libpulsar.so) was built, if the other libraries like the Python and Node.js wrappers use the C++ library, the client_version field could not be modified. It causes an issue that when the Python client or Node.client connectes to the broker, the version info is still the C++ client's version because these clients are based on the C++ client.

Modifications

  • Add setClientVersion and getClientVersion functions to set or get the client_version field in CommandConnect.
  • To not bring unnecessary mutex overhead, no mutexes are used to protect the access to the global version variable. So the setClientVersion method should be called before creating any Client object.
  • Remove the template to generate Version.h.

### Motivation

Currently C++ client generates a `Version.h` based on the `Version.h.in`
in the `templates` directory when compiling. The header includes two
macros:
- `PULSAR_VERSION`: Never used
- `PULSAR_VERSION_STR`: Used as the `client_version` field in
  `CommandConnect`

Once the library (e.g. `libpulsar.so`) was built, if the other libraries
like the Python and Node.js wrappers use the C++ library, the
`client_version` field could not be modified. It causes an issue that
when the Python client or Node.client connectes to the broker, the
version info is still the C++ client's version because these clients are
based on the C++ client.

### Modifications

- Add `setClientVersion` and `getClientVersion` functions to set or get
  the `client_version` field in `CommandConnect`.
- To not bring unnecessary mutex overhead, no mutexes are used to
  protect the access to the global version variable. So the
  `setClientVersion` method should be called before creating any
  `Client` object.
- Remove the template to generate `Version.h`.
@BewareMyPower BewareMyPower added the enhancement New feature or request label Mar 3, 2023
@BewareMyPower BewareMyPower added this to the 3.2.0 milestone Mar 3, 2023
@BewareMyPower BewareMyPower self-assigned this Mar 3, 2023
@BewareMyPower
Copy link
Contributor Author

Before reviewing this PR, I think we need to start a proposal since it introduces a new API. Here is the discussion before: https://lists.apache.org/thread/n59k537fhthjnzkfxtc2p4zk4l0cv3mp

I'm going to start a formal PIP soon.

@BewareMyPower BewareMyPower marked this pull request as draft March 3, 2023 15:25
@massakam
Copy link
Contributor

massakam commented Mar 9, 2023

Is it possible that users are including Version.h and using these macros? If so, removing these macros may be a breaking change.

  • PULSAR_VERSION
  • PULSAR_VERSION_STR

FYI, these macros were added in apache/pulsar#12769. These were originally intended to be used by the Pulsar Node.js client, but ended up not being used.
apache/pulsar-client-node#252 (comment)

@BewareMyPower
Copy link
Contributor Author

Is it possible that users are including Version.h and using these macros?

Oh I see. It will be installed from pre-built binaries. I only checked make install does not install this header because I didn't use the project directory as the CMake directory.

And from the discussion here, we might support configuring the client version directly on a Client object. So I close this PR first.

@BewareMyPower BewareMyPower deleted the bewaremypower/client-version branch March 10, 2023 02:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants