Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add CRC32 checkSum control #285

Merged
merged 10 commits into from
Sep 7, 2023
Merged

Add CRC32 checkSum control #285

merged 10 commits into from
Sep 7, 2023

Conversation

Gsantomaggio
Copy link
Member

@Gsantomaggio Gsantomaggio commented Sep 3, 2023

  • Small fix application properties in case of byte value on reading.
  • Closes Implement CRC control #19
  • possibility to enable/disable the CRC32 control with the Crc32 interface
  • The user is free to use the Crc implementation

RawConsumer:

        
      private readonly ICrc32 _crc32 = new MyCrcImplentation()
        new RawConsumerConfig(stream)
              {
                  Crc32 = _crc32,
              

By default, the CRC control is disabled to be compatible with the correct version.
Enable the CRC control could reduce the performances in some cases since it has to calculate the CRC for each chunk.

It is recommended to enable it.

The documentation needs to be included; once this is approved, we will update the documentation.

Fix application propriets in case of byte value

Signed-off-by: Gabriele Santomaggio <G.santomaggio@gmail.com>
@codecov
Copy link

codecov bot commented Sep 3, 2023

Codecov Report

Patch coverage: 67.85% and project coverage change: -0.21% ⚠️

Comparison is base (8a85779) 93.04% compared to head (c93b41d) 92.84%.

❗ Current head c93b41d differs from pull request most recent head 494a0eb. Consider uploading reports for the commit 494a0eb to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #285      +/-   ##
==========================================
- Coverage   93.04%   92.84%   -0.21%     
==========================================
  Files         110      112       +2     
  Lines        9640     9687      +47     
  Branches      764      769       +5     
==========================================
+ Hits         8970     8994      +24     
- Misses        511      531      +20     
- Partials      159      162       +3     
Files Changed Coverage Δ
RabbitMQ.Stream.Client/Client.cs 90.46% <0.00%> (ø)
RabbitMQ.Stream.Client/ClientExceptions.cs 58.00% <0.00%> (-7.91%) ⬇️
RabbitMQ.Stream.Client/Connection.cs 88.65% <ø> (ø)
RabbitMQ.Stream.Client/InternalExceptions.cs 0.00% <0.00%> (ø)
RabbitMQ.Stream.Client/Deliver.cs 85.89% <11.11%> (-10.05%) ⬇️
...itMQ.Stream.Client/AMQP/AmqpWireFormattingWrite.cs 83.27% <100.00%> (+0.06%) ⬆️
RabbitMQ.Stream.Client/IConsumer.cs 100.00% <100.00%> (ø)
RabbitMQ.Stream.Client/RawConsumer.cs 86.19% <100.00%> (+0.63%) ⬆️
RabbitMQ.Stream.Client/RawSuperStreamConsumer.cs 94.26% <100.00%> (ø)
RabbitMQ.Stream.Client/Reliable/Consumer.cs 100.00% <100.00%> (ø)
... and 6 more

... and 1 file with indirect coverage changes

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

replace copy with TryCopyTo to check if there are enough data to read. This
will raise an Exception.

Signed-off-by: Gabriele Santomaggio <G.santomaggio@gmail.com>
@jonnepmyra
Copy link
Contributor

Currently evaluating this PR in our dev environments, looks good so far, no unexpected behaviours found, will continue to monitor for a couple of days

@michaelklishin
Copy link
Member

@jonnepmyra thank you for following up, we really appreciate the feedback!

Signed-off-by: Gabriele Santomaggio <G.santomaggio@gmail.com>
the CRC32 control.
Add some crc32 test.
Enable the CRC32 control on some consumer test

Signed-off-by: Gabriele Santomaggio <G.santomaggio@gmail.com>
Signed-off-by: Gabriele Santomaggio <G.santomaggio@gmail.com>
@Gsantomaggio Gsantomaggio marked this pull request as ready for review September 6, 2023 13:41
@lukebakken lukebakken self-requested a review September 6, 2023 14:23
@lukebakken lukebakken added this to the 1.7.0 milestone Sep 6, 2023
Copy link
Contributor

@lukebakken lukebakken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than code and maintain our own Crc32 implementation, should we allow the user to provide one if they want this feature? i.e. if you want CRC32 checking, you must include one of these libraries with your application -

@Gsantomaggio
Copy link
Member Author

The idea was to reduce as much as possible the external dependencies.
If we want to add a library I would add ttps://www.nuget.org/packages/System.IO.Hashing/6.0.0

remove the old crc32 class

Signed-off-by: Gabriele Santomaggio <G.santomaggio@gmail.com>
remove the old crc32 class

Signed-off-by: Gabriele Santomaggio <G.santomaggio@gmail.com>
@lukebakken lukebakken self-requested a review September 7, 2023 12:19
lukebakken added a commit that referenced this pull request Sep 7, 2023
If a library user wishes to have Crc32 checking, they must implement the
`ICrc32` interface and set an implementation on the configuration.

References:
* #19
* #285
@lukebakken lukebakken mentioned this pull request Sep 7, 2023
lukebakken added a commit that referenced this pull request Sep 7, 2023
If a library user wishes to have Crc32 checking, they must implement the
`ICrc32` interface and set an implementation on the configuration.

References:
* #19
* #285
If a library user wishes to have Crc32 checking, they must implement the
`ICrc32` interface and set an implementation on the configuration.

References:
* #19
* #285
@lukebakken lukebakken mentioned this pull request Sep 7, 2023
Copy link
Contributor

@lukebakken lukebakken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @Gsantomaggio !!!

@Gsantomaggio Gsantomaggio merged commit 53f011e into main Sep 7, 2023
2 checks passed
@Gsantomaggio Gsantomaggio deleted the check_chunk_size branch September 7, 2023 13:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement CRC control
4 participants