You are viewing a plain text version of this content. The canonical link for it is here.

Posted to commits@pulsar.apache.org by "BewareMyPower (via GitHub)" <gi...@apache.org> on 2023/03/03 10:35:27 UTC

[GitHub] [pulsar-client-cpp] BewareMyPower opened a new pull request, #208: Support configure global level client version string

BewareMyPower opened a new pull request, #208:
URL: https://github.com/apache/pulsar-client-cpp/pull/208

### 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`.

--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar-client-cpp] massakam commented on pull request #208: Support configure global level client version string

Posted by "massakam (via GitHub)" <gi...@apache.org>.

massakam commented on PR #208:
URL: https://github.com/apache/pulsar-client-cpp/pull/208#issuecomment-1461256213

   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 https://github.com/apache/pulsar/pull/12769. These were originally intended to be used by the Pulsar Node.js client, but ended up not being used.
   https://github.com/apache/pulsar-client-node/pull/252#discussion_r1040405072


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar-client-cpp] BewareMyPower commented on pull request #208: Support configure global level client version string

Posted by "BewareMyPower (via GitHub)" <gi...@apache.org>.

BewareMyPower commented on PR #208:
URL: https://github.com/apache/pulsar-client-cpp/pull/208#issuecomment-1463104573

   > 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](https://lists.apache.org/thread/65cf7w76tt23sbsjnr8rpfxqf1nt9s9l), we might support configuring the client version directly on a `Client` object. So I close this PR first.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar-client-cpp] BewareMyPower commented on pull request #208: Support configure global level client version string

Posted by "BewareMyPower (via GitHub)" <gi...@apache.org>.

BewareMyPower commented on PR #208:
URL: https://github.com/apache/pulsar-client-cpp/pull/208#issuecomment-1453323500

   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.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar-client-cpp] BewareMyPower closed pull request #208: Support configure global level client version string

Posted by "BewareMyPower (via GitHub)" <gi...@apache.org>.

BewareMyPower closed pull request #208: Support configure global level client version string
URL: https://github.com/apache/pulsar-client-cpp/pull/208


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org