Skip to content

Only log ErrorCategory::INTERNAL errors#676

Merged
mehertz merged 2 commits into
masterfrom
only-log-internal
Jul 31, 2023
Merged

Only log ErrorCategory::INTERNAL errors#676
mehertz merged 2 commits into
masterfrom
only-log-internal

Conversation

@mehertz

@mehertz mehertz commented Jul 31, 2023

Copy link
Copy Markdown
Collaborator

Reference Issues/PRs

Fixes #675

What does this implement/fix? How does it work (high level)? Highlight notable design decisions.

Only ErrorCode::INTERNAL will be logged to standard error.

Otherwise exceptions will be raised to be dealt with by the caller.

Any other comments?

I'm unsure if this will cause some messages to be lost if the exception is not percolated upwards.

Checklist

Checklist for code changes...
  • Have you updated the relevant docstrings and documentation?
  • Is this contribution tested against all ArcticDB's features?
  • Do all exceptions introduced raise appropriate error messages?
  • Are API changes highlighted in the PR description?
  • Is the PR labelled as enhancement or bug so it appears in autogenerated release notes?

@mehertz mehertz added the bug Something isn't working label Jul 31, 2023
@mehertz mehertz marked this pull request as ready for review July 31, 2023 14:56
@mehertz mehertz merged commit 9b7d877 into master Jul 31, 2023
@poodlewars poodlewars deleted the only-log-internal branch October 25, 2023 17:50
phoebusm added a commit that referenced this pull request Jun 12, 2026
#### Reference Issues/PRs
<!--Example: Fixes #1234. See also #3456.-->
Fix `test_deep_nesting_metastruct_size_over_limit` and
`test_deep_nesting_metastruct_size_under_limit`
Culprit: `msgpack 1.2.0` has just released. It includes [Raise
DEFAULT_RECURSE_LIMIT from 511 to 1024 by Copilot · Pull Request #676 ·
msgpack/msgpack-python](msgpack/msgpack-python#676)
which changes `DEFAULT_RECURSE_LIMIT` from 511 to 1024
`python 3.9` is unaffected as the new msgpack version has dropped the
support

#### What does this implement or fix?
Hard-pin the arcticdb limit to `511` as msgpack <1.2.0
#### Any other comments?
msgpack <1.2.0 has the recurse limit set at 511. arcticdb imports the
limit from msgpack to calculate the metastruct layer limit
msgpack 1.2.0 has raised limit from 511 to 1024. Since reconstructing
the metastruct from is done by recursion, increasing the no. of layers
in the metastruct will make arcticdb hit the python recursion limit.
#### Checklist

<details>
  <summary>
   Checklist for code changes...
  </summary>
 
- [ ] Have you updated the relevant docstrings, documentation and
copyright notice?
- [ ] Is this contribution tested against [all ArcticDB's
features](../docs/mkdocs/docs/technical/contributing.md)?
- [ ] Do all exceptions introduced raise appropriate [error
messages](https://docs.arcticdb.io/error_messages/)?
 - [ ] Are API changes highlighted in the PR description?
- [ ] Is the PR labelled as enhancement or bug so it appears in
autogenerated release notes?
</details>

<!--
Thanks for contributing a Pull Request to ArcticDB! Please ensure you
have taken a look at:
- ArcticDB's Code of Conduct:
https://github.com/man-group/ArcticDB/blob/master/CODE_OF_CONDUCT.md
- ArcticDB's Contribution Licensing:
https://github.com/man-group/ArcticDB/blob/master/docs/mkdocs/docs/technical/contributing.md#contribution-licensing
-->
IvoDD added a commit that referenced this pull request Jun 17, 2026
#### Reference Issues/PRs
<!--Example: Fixes #1234. See also #3456.-->
Fix `test_deep_nesting_metastruct_size_over_limit` and
`test_deep_nesting_metastruct_size_under_limit`
Culprit: `msgpack 1.2.0` has just released. It includes [Raise
DEFAULT_RECURSE_LIMIT from 511 to 1024 by Copilot · Pull Request #676 ·
msgpack/msgpack-python](msgpack/msgpack-python#676)
which changes `DEFAULT_RECURSE_LIMIT` from 511 to 1024 `python 3.9` is
unaffected as the new msgpack version has dropped the support

#### What does this implement or fix?
Hard-pin the arcticdb limit to `511` as msgpack <1.2.0 #### Any other
comments?
msgpack <1.2.0 has the recurse limit set at 511. arcticdb imports the
limit from msgpack to calculate the metastruct layer limit msgpack 1.2.0
has raised limit from 511 to 1024. Since reconstructing the metastruct
from is done by recursion, increasing the no. of layers in the
metastruct will make arcticdb hit the python recursion limit. ####
Checklist

<details>
  <summary>
   Checklist for code changes...
  </summary>
 
- [ ] Have you updated the relevant docstrings, documentation and
copyright notice?
- [ ] Is this contribution tested against [all ArcticDB's
features](../docs/mkdocs/docs/technical/contributing.md)?
- [ ] Do all exceptions introduced raise appropriate [error
messages](https://docs.arcticdb.io/error_messages/)?
 - [ ] Are API changes highlighted in the PR description?
- [ ] Is the PR labelled as enhancement or bug so it appears in
autogenerated release notes?
</details>

<!--
Thanks for contributing a Pull Request to ArcticDB! Please ensure you
have taken a look at:
- ArcticDB's Code of Conduct:
https://github.com/man-group/ArcticDB/blob/master/CODE_OF_CONDUCT.md
- ArcticDB's Contribution Licensing:
https://github.com/man-group/ArcticDB/blob/master/docs/mkdocs/docs/technical/contributing.md#contribution-licensing
-->

#### Reference Issues/PRs
<!--Example: Fixes #1234. See also #3456.-->

#### What does this implement or fix?

#### Any other comments?

#### Checklist

<details>
  <summary>
   Checklist for code changes...
  </summary>
 
- [ ] Have you updated the relevant docstrings, documentation and
copyright notice?
- [ ] Is this contribution tested against [all ArcticDB's
features](../docs/mkdocs/docs/technical/contributing.md)?
- [ ] Do all exceptions introduced raise appropriate [error
messages](https://docs.arcticdb.io/error_messages/)?
 - [ ] Are API changes highlighted in the PR description?
- [ ] Is the PR labelled as enhancement or bug so it appears in
autogenerated release notes?
</details>

<!--
Thanks for contributing a Pull Request to ArcticDB! Please ensure you
have taken a look at:
- ArcticDB's Code of Conduct:
https://github.com/man-group/ArcticDB/blob/master/CODE_OF_CONDUCT.md
- ArcticDB's Contribution Licensing:
https://github.com/man-group/ArcticDB/blob/master/docs/mkdocs/docs/technical/contributing.md#contribution-licensing
-->

Co-authored-by: Phoebus Mak <61957902+phoebusm@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

read_batch logs a warning when data is missing

2 participants