[ADDED] Pull consumer priority groups (nats-server v.2.11 feature) by levb · Pull Request #869 · nats-io/nats.c

levb · 2025-04-17T16:51:51Z

See https://github.com/nats-io/nats-architecture-and-design/blob/main/adr/ADR-42.md and nats-io/nats.go#1826

…oups

levb · 2025-04-17T16:55:23Z

+    if (strncmp(val, HDR_STATUS_PIN_ID_MISMATCH, HDR_STATUS_LEN) == 0)
+        return NATS_PIN_ID_MISMATCH;
+
+    if (strncmp(val, HDR_STATUS_BAD_REQUEST, HDR_STATUS_LEN) == 0)


HDR_STATUS_BAD_REQUEST now returns NATS_INVALID_ARG, before it was defaulting to NATS_ERR. This affected some existing tests.

Do we need to check the string or checking that it is "400" is enough?

The string - you mean the error/status string? We always set the (fetch) status to NATS_INVALID_ARG (for the lack of a NATS_BAD_REQUEST, but isn't it the same thing really?) and set the last error string to the description.

codecov · 2025-04-17T16:58:44Z

Codecov Report

Attention: Patch coverage is 59.39850% with 54 lines in your changes missing coverage. Please review.

Project coverage is 70.25%. Comparing base (1553d4a) to head (cac9643).
Report is 59 commits behind head on main.

Files with missing lines	Patch %	Lines
src/jsm.c	64.70%	8 Missing and 16 partials ⚠️
src/js.c	46.87%	11 Missing and 6 partials ⚠️
src/util.c	61.11%	2 Missing and 5 partials ⚠️
src/dispatch.c	64.28%	1 Missing and 4 partials ⚠️
src/buf.c	0.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #869      +/-   ##
==========================================
+ Coverage   68.71%   70.25%   +1.54%     
==========================================
  Files          39       47       +8     
  Lines       15207    15550     +343     
  Branches     3143     3202      +59     
==========================================
+ Hits        10449    10925     +476     
+ Misses       1700     1549     -151     
- Partials     3058     3076      +18

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

kozlovic · 2025-04-17T21:58:16Z

+    if (strncmp(val, HDR_STATUS_PIN_ID_MISMATCH, HDR_STATUS_LEN) == 0)
+        return NATS_PIN_ID_MISMATCH;
+
+    if (strncmp(val, HDR_STATUS_BAD_REQUEST, HDR_STATUS_LEN) == 0)


Do we need to check the string or checking that it is "400" is enough?

kozlovic

Changes I asked for looks good, so on my end I will LGTM, but there is a typo that @mtmk noticed that would need to be fixed. We could also revisit the need for the bitmap to check for valid term.

kozlovic · 2025-04-18T15:15:30Z

+    for (int i=0; term[i] != '\0'; ++i) {
        if (i >= 16)
            return nats_setError(NATS_INVALID_ARG, "%s must not be longer than 16 characters", name);



I see that @mtmk posted a comment on the _limited_term_allowed_bitmap, we could also simply do the checks down below and make sure c is valid without the use of such function.

kozlovic

LGTM

… (nats-server v.2.11 feature) (#869)

@mtmk

Release notes will be: This release contains some breaking changes. See the "Changed" section below. * Build * Disable NATS Streaming by default by @mtmk in #770 * TLS * Require OpenSSL 1.1.1+ to compile. Removed the `NATS_BUILD_TLS_USE_OPENSSL_1_1_API` CMake variable by @kozlovic in #905 * The option `natsOptions_SetSSLVerificationCallback` signature was changed to replace the use of `SSL_verify_cb` (which required OpenSSL dependency in the `nats.h` file), to the new callback `natsSSLVerifyCb`. See documentation of `natsSSLVerifyCb` to see the cast needed to compile with this new header file by @kozlovic in #908 * Modification of a `natsOptions` object if it has TLS/SSL configuration and is actively used by connections will now return a `NATS_ILLEGAL_STATE` by @kozlovic in #912 * Options * Ability to load the trusted CA certificates from a directory using the new option `natsOptions_LoadCATrustedCertificatesPath` by @kerbert101 in #862 * Ability to connect via HTTP proxy for instance by adding a proxy connection handler using the new option `natsOptions_SetProxyConnHandler` by @wolfkor in #871 and @kozlovic in #897 * Ability to load the certificate chain and key from a file on every connection attempt using the new option `natsOptions_LoadCertificatesChainDynamic` by @Matus-p in #901 * Ability to perform concurrent TLS handshakes that may improve time it takes for concurrent connections to be established using the new option `natsOptions_AllowConcurrentTLSHandshakes ` by @kozlovic in #914. Issue was reported by @yanyongcheng in #899 * JetStream * Per-message TTL support (a NATS Server v2.11 feature) by @levb in #863 * Pull consumer priority groups (a NATS Server v2.11 feature) by @levb in #869 * ObjectStore support by @kozlovic in #902. Thanks to @jfflynn41 and @alex1891 for the feedback in #876 * JetStream * Handling of publish asynchronous timeouts by @kozlovic in #886. Issue reported by @yanyongcheng in #880 * Timer insertion by @kozlovic in #883. Issue reported by @yanyongcheng in #881 * EventLoop: * Handling of possible failure on initial attach by @kozlovic in #918 * LibEvent: `natsConnection_Close()` not closing the TCP connection by @kozlovic in #882. Issue was reported by @yanyongcheng in #879 * Libuv: Possible crash if connection is destroyed while receiving data by @kozlovic in #889. Issue was reported by @yanyongcheng in #888 * KeyValue * Keys, History or watcher's next may incorrectly return `NATS_TIMEOUT` by @kozlovic in #917/ Issue was reported by @ArashPartow in #916 * MicroServices: * Wrong marshaling of `average_processing_time` by @kozlovic in #892. Issue was reported by @Archie3d in #890 * Statistics error was always incremented by @kozlovic in #894. Issue was reported by @Archie3d in #893 * TLS * Unknown type name `SSL_verify_cb` by @kozlovic in #878. Issue was reported by @philipfoulkes in #877 * Initialization and cleanup code related to OpenSSL was removed since it was deprecated for versions post OpenSSL 1.1. A cleanup function pertinent to 1.1+ code was possibly causing a problem. By @kozlovic in #905. Issue was reported by @vdeters in #904 * Possible hang during handshake by @kozlovic in #907. Issue was reported by @etrochim in #906 * Protect calls to `SSL_read` and `SSL_write` with a mutex. Since the same `SSL` object is shared between different threads, the OpenSSL library requires a mutex to be used by @kozlovic in #913 * Memory allocation check by @wooffie in #868 * Add missing status text string by @oldnick85 in #872 and @kozlovic in #874 (the issue was not present in any published release and was introduced in #869) * Parsing of message headers with `NULL` or all-whitespace values by @habbbe in #873 * Removed some unused code related to handling of responses and added custom inbox with very long prefix test by @kozlovic in #885. Issue was reported by @yanyongcheng in #884 * Connection drain could cause missed reply and/or a 100ms delay by @kozlovic in #915. Issue was reported by @T-Maxxx in #911 * Build * Deprecated Ubuntu 20.04 in GitHub actions by @levb in #864 * Removed the older compiler jobs by @levb in #865 * Fixed Windows build to use the NATS Server main branch by @levb in #866 * @kerbert101 made their first contribution in #862 * @habbbe made their first contribution in #873 * @wolfkor made their first contribution in #871 * @Matus-p made their first contribution in #901 Signed-off-by: Ivan Kozlovic <ivan@synadia.com>

[ADDED] Pull consumer priority groups (nats-server v.2.11 feature)

154e6f4

See https://github.com/nats-io/nats-architecture-and-design/blob/main/adr/ADR-42.md and nats-io/nats.go#1826

levb requested review from Jarema, kozlovic and mtmk April 17, 2025 16:51

Merge branch 'main' of github.com:nats-io/nats.c into lev-consumer-gr…

396b5c6

…oups

levb commented Apr 17, 2025

View reviewed changes

levb marked this pull request as ready for review April 17, 2025 17:06

kozlovic requested changes Apr 17, 2025

View reviewed changes

PR feedback

79495a8

levb requested a review from kozlovic April 18, 2025 14:15

mtmk reviewed Apr 18, 2025

View reviewed changes

Comment thread src/util.c Outdated

mtmk reviewed Apr 18, 2025

View reviewed changes

Comment thread src/util.c Outdated

kozlovic requested changes Apr 18, 2025

View reviewed changes

Lev Brouk added 2 commits April 18, 2025 08:49

PR feedback: fixed the bitmap and added a test

39a1fc2

PR feedback: fixed the error message

cac9643

levb requested review from kozlovic and mtmk April 18, 2025 16:16

kozlovic approved these changes Apr 18, 2025

View reviewed changes

levb merged commit a1dc0da into main Apr 18, 2025

levb deleted the lev-consumer-groups branch April 18, 2025 16:44

github-actions Bot pushed a commit that referenced this pull request Apr 18, 2025

[skip ci] Update docs: a1dc0da: [ADDED] Pull consumer priority groups…

8ab0f03

… (nats-server v.2.11 feature) (#869)

kozlovic mentioned this pull request May 6, 2025

fix: add missing statusText string #872

Merged

Uh oh!

Conversation

levb commented Apr 17, 2025

Uh oh!

levb Apr 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kozlovic Apr 17, 2025

Choose a reason for hiding this comment

Uh oh!

levb Apr 18, 2025

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Apr 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

kozlovic Apr 17, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kozlovic left a comment

Choose a reason for hiding this comment

Uh oh!

kozlovic Apr 18, 2025

Choose a reason for hiding this comment

Uh oh!

kozlovic left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

levb Apr 17, 2025 •

edited

Loading

codecov Bot commented Apr 17, 2025 •

edited

Loading