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

irmin-pack: disallow split when gc is disallowed #2175

Merged
merged 4 commits into from
Jan 31, 2023

Conversation

metanivek
Copy link
Member

This PR addresses an issue found while testing the latest octez release candidate.

The ultimate result of the this PR is to disallow split if GC is disallowed, but also includes a commit that fixes the issue in case it is allowed in the future.

  • Raise Split_disallowed exception when calling split on a a store that does not allow it
  • Add Store.is_split_allowed to check whether split can be called without raising an exception
  • The commit with the fix for the initial issue: 51f97ce. The issue was: when attempting to call split on a store that has status From_v1_v2_post_upgrade, it would raise an Inconsistent_store exception when attempting to re-open the first chunk as read-only because the end_poff calculation did not subtract the dead_header_size. This exception was false; no actual on-disk data corruption had occurred. Thanks to @art-w for the help in investigation and discovery of this root cause. 🙇

Copy link
Contributor

@art-w art-w left a comment

Choose a reason for hiding this comment

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

Thanks a lot for taking care of this! It looks good :)

The value stored in the control file is *not* inclusive of the dead
header size, so we must subtract it from the on-disk size before passing
to Append_only_file's open function. Otherwise, the consistency check is
not correct and will falsely claim the chunk is inconsistent.
Split will raise an exception if it is not allowed. Also, expose a
function for checking if it is enabled, which is the same as GC being
enabled or not.
@metanivek metanivek merged commit 22b2863 into mirage:main Jan 31, 2023
@metanivek metanivek deleted the fix_split_from_v2_always branch January 31, 2023 19:50
metanivek added a commit to metanivek/opam-repository that referenced this pull request Apr 21, 2023
…min-pack, irmin-pack-tools, irmin-mirage, irmin-mirage-graphql, irmin-mirage-git, irmin-http, irmin-graphql, irmin-git, irmin-fs, irmin-containers, irmin-cli, irmin-chunk and irmin-bench (3.7.0)

CHANGES:

### Added

- **irmin**
  - Add `Conf.pp` and `Conf.equal` to print and compare configuration values
    (mirage/irmin#2227, @samoht)
  - Add a `clear` optional arguments to all function that adds a new commit:
    `Commit.v`, `set`, `set_tree`, `remove`, `test_and_set`,
    `test_and_set_tree`, `test_set_and_get`, `test_set_and_get_tree`, `merge`,
    `merge_tree` and `with_tree`. This new argument allows to control whether
    the tree caches are cleared up after objects are exported to disk during
    the commit. (mirage/irmin#2225, @samoht)

- **irmin-pack**
  - Add configuration option, `lower_root`, to specify a path for archiving data
    during a GC. (mirage/irmin#2177, @metanivek)
  - Add `is_split_allowed` to check if a store allows split. (mirage/irmin#2175, @metanivek)
  - Add `add_volume` to allow creating new empty volume in lower layer. (mirage/irmin#2188,
    @metanivek)
  - Add a `behaviour` function to the GC to check wether the GC will archive or
    delete data. (mirage/irmin#2190, @Firobe)
  - Add a migration on `open_rw` to move the data to the `lower_root` if
    the configuration was enabled (mirage/irmin#2205, @art-w)

### Changed

- **irmin**
  - Expose type equality for `Schema.Info` to avoid defining the `info` function
    multiple times when using similar stores (mirage/irmin#2189, mirage/irmin#2193, @samoht)
- **irmin-pack**
  - GC now changes its behaviour depending on the presence of a lower layer.
    (mirage/irmin#2190, @Firobe)
  - Split now raises an exception if it is not allowed. It is not allowed on
    stores that do not allow GC. (mirage/irmin#2175, @metanivek)
  - GC now supports stores imported V1/V2 stores, in presence of a lower layer
    only. (mirage/irmin#2190, @art-w, @Firobe)
  - Upgrade on-disk format to version 5. (mirage/irmin#2184, @metanivek)
  - Archive to lower volume does not copy orphaned commits. (mirage/irmin#2215, @art-w)

### Fixed
- **irmin-pack**
  - Unhandled exceptions in GC worker process are now reported as a failure
    (mirage/irmin#2163, @metanivek)
  - Fix the silent mode for the integrity checks. (mirage/irmin#2179, @icristescu)
  - Fix file descriptor leak caused by `mmap`. (mirage/irmin#2232, @art-w)
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.

None yet

2 participants