Skip to content

AVRO-3345: Made Magic readonly in DataFileConstants #1495

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.

Sign up for GitHub

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

Merged
martin-g merged 1 commit into from
Feb 1, 2022
Merged

AVRO-3345: Made Magic readonly in DataFileConstants #1495

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 7 additions & 7 deletions lang/csharp/src/apache/main/File/DataFileConstants.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,6 @@ namespace Avro.File
/// <summary>
/// Constants used in data files.
/// </summary>
[System.Diagnostics.CodeAnalysis.SuppressMessage("Design",
"CA1052:Static holder types should be Static or NotInheritable",
Justification = "Maintain public API")]
Copy link
Member

@martin-g martin-g Feb 1, 2022

Choose a reason for hiding this comment

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

Is this change related to the Magic constant ?

Copy link
Contributor Author

@KyleSchoonover KyleSchoonover Feb 1, 2022

Choose a reason for hiding this comment

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

It was, but it's an old rule. The way you get rid of it, is just make the static property readonly. Allows you to essentially make a static property a constant. The reason you do this is because constants are basic types and not a reference type like an array.

public class DataFileConstants
{
/// <summary>
Expand Down Expand Up @@ -64,10 +61,13 @@ public class DataFileConstants
/// <summary>
/// Magic bytes at the beginning of an Avro data file.
/// </summary>
public static byte[] Magic = { (byte)'O',
(byte)'b',
(byte)'j',
Version };
public static readonly byte[] Magic =
Copy link
Member

@martin-g martin-g Feb 1, 2022

Choose a reason for hiding this comment

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

The looks like an API change but I guess no one is supposed to change the magic bytes

Copy link
Contributor Author

@KyleSchoonover KyleSchoonover Feb 1, 2022

Choose a reason for hiding this comment

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

The change was formatting.

Copy link
Member

@martin-g martin-g Feb 1, 2022

Choose a reason for hiding this comment

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

The change was formatting.

What do you mean ?

AFAIU readonly makes Magic a constant, like final in Java.
Until now a user can change the value of Magic in his/her application. After this PR it won't be possible anymore. And such application will fail to compile. This is what I meant by API change.
But I agree that no one is supposed to change the magic bytes.

Copy link
Contributor Author

@KyleSchoonover KyleSchoonover Feb 1, 2022

Choose a reason for hiding this comment

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

You are correct. Do we want the backwards support to allow the change? It appears the intention was that it is a constant hence the readonly static. Like with most things, if it is available someone is using it. The other option is I can mark this obsolete, and add the remark for a future change that it be made readonly.

Copy link
Member

@martin-g martin-g Feb 1, 2022

Choose a reason for hiding this comment

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

I am 99% sure no one is setting custom value for the Magic field. So I am OK to merge it as is! But only for 1.12.0!
If someone complains that we broke his/her build then we can revert if (s)he gives us a really convincing reason!

{
(byte)'O',
(byte)'b',
(byte)'j',
Version,
};

/// <summary>
/// Hash code for the null codec.
Expand Down