Part 7 - Zarr consolidated metadata

[mannreis]

This PR follows up on #3224 :

• Adds mode control parameters csl,consolidate,consolidated to select the use of consolitated metadata.
• Adds directives to control the via environment varibles the use of consolidated metadata.
• Modifies the NCZMD_set_metadata_handler to select the handler based on the settings, and presence of consolidated metadata.
• Extends the tests to confirm the correct usage of consolidated metadata

[DennisHeimbigner]

Ok, I have reviewed my initial notes are as follows:

• csl-1-5 seem ok.

• csl-6:

  • in NCZMD_v2_csl_list_nodes, you appear to be trying to parse keys yourself. There are some key split/join functions (see include ncutil.h) that might be useful here.
  • Otherwise ok.

• csl-7:

  • using three different mode keys -- csl,consolidate,consolidated -- to signal consolidated format is probably excessive. You should probably settle on one.
  • This PR did not pass all tests, so I fixed it.
  • otherwise ok

[mannreis]

Thanks for the feedback! I will take a look at the recommendations during this week.

I was waiting for the reviews of the previous branches to do a final rebase, that would have fixed the tests. Now it's more tricky but definitely doable. Let me know if the plan is still to merge one PR at the time. Or simply this one once it contains all the necessary changes

[DennisHeimbigner]

I will defer to you on that choice; either is ok with me.

[dopplershift]

I will defer to you on that choice; either is ok with me.

If everything is going in all merged together, I'm not sure what the point of breaking things up into separate PRs was. I'd recommend merging 1 PR at a time.

[mannreis]

Thanks for stating your preference @dopplershift. I agreed! We go with merging the individual PRs in order.

As the first one gets merged I'll git pull --rebase upstream main && git push --force the next one so the history will be cleaner. So you might need to re approve more times after the last push so I recommends approving and merging one at time.
CC: @WardF @DennisHeimbigner

[mannreis]

@DennisHeimbigner I've force pushed to this branch to align all 7 PRs! Please pull --rebase if you intend to add anything.

I'll just go over the overall feedback:

• csl-1-5 seem ok.

I've fixed a memory leak via 05b4732

• csl-6:

  • in NCZMD_v2_csl_list_nodes, you appear to be trying to parse keys yourself. There are some key split/join functions (see include ncutil.h) that might be useful here.

0b8a548 addresses this by using NC_split_delim

• csl-7:

  • using three different mode keys -- csl,consolidate,consolidated -- to signal consolidated format is probably excessive. You should probably settle on one.

a48f9d4 and 7d1b361 address your last suggestion to keep only mode=consolidated

• This PR did not pass all tests, so I fixed it.

You can see now that my commit b0b74a5 (in Part 5 zarr-csl-5) is equalent to what you added here in part 7 (e938fbd + ff3d0f9). So we can clean this after part 5 or 6 is are merged.

Many thanks for your prompt review! And I'm sorry for this git puzzle

CC: @WardF

[DennisHeimbigner]

I will defer any additional changes on my part since I have some other largely independent PRs that I will need to integrate with your changes. I think it is best to just move forward so you can begin work on your original remote server system.

[mannreis]

Ok! Then I'd appreciate it if you could give your final approval to the PRs. Thanks!

The merge-base changed after approval.

The merge-base changed after approval.

[WardF]

I have approved PR's 1-6 and am re-running the tests on this final PR now. I'm leaving for AGU and I know the holidays are coming up so I want to get this merged in. I'm still getting caught up, but things look good on my end, I'll merge this after the final tests pass here :)

[mannreis]

Thanks a lot for the speedy review! Take your time for the final PR and don't hesitate if you need more clarifications/discussion.

[DennisHeimbigner]

Manuel- Will you now be able to move forward on your server now?

[mannreis]

Manuel- Will you now be able to move forward on your server now?

It unblocks the path for what we'd like to achieve! But it has nothing to do with any server

Let me try clarify any misunderstanding.

I'm not working on a server.
In the past I've shared by email a list of endpoints we'd like to see working with the upstream netcdf-c.
That wish still stands, and this PR is the first big step towards that.
But most importantly, it offers a huge performance improvement and closes #2987

I'll create a new discussion to where we can move to.