check-crash: Improve io_flush check with target_name#92
Merged
kou merged 1 commit intogroonga:mainfrom Jan 26, 2026
Merged
Conversation
There was a problem hiding this comment.
Pull request overview
This PR aligns check-crash’s handling of io_flush with the Groonga io_flush documentation, specifically around target_name usage and the recursive parameter, and extends flushing detection to table_create and column_create commands. It adds comprehensive fixtures and tests to verify flushed/unflushed detection for load, delete, truncate, table_create, and column_create under various io_flush parameter combinations.
Changes:
- Update
check_io_flushlogic to treatio_flushwithtarget_nameas effective only whenrecursive=dependent, and to mark objects fromtable_createandcolumn_createas flushed when appropriate. - Add new “flushed” and “unflushed” query log fixtures for
load,delete,truncate,table_create, andcolumn_createcoveringrecursive=yes/noandtarget_namecombinations. - Expand
test-check-crash.rbto use the new fixtures, exercising both flushed and unflushed paths for the new semantics, while keeping the existing crash summary expectations.
Reviewed changes
Copilot reviewed 2 out of 27 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/groonga-query-log/command/check-crash.rb | Adjusts check_io_flush to only clear unflushed stats when io_flush has target_name and recursive=dependent, and to handle table_create (name) and column_create (table.name) targets as flushed objects. |
| test/command/test-check-crash.rb | Adds data-driven test cases for flushed and unflushed scenarios across commands (load, delete, truncate, table_create, column_create) with io_flush variations; minor naming/label typos remain as nits. |
| test/fixtures/check-crash/query/truncate/unflushed/target-name-recursive-yes.log | Unflushed truncate scenario with io_flush?target_name=Data&recursive=yes, used to assert that non-dependent recursion with target_name does not clear unflushed stats. |
| test/fixtures/check-crash/query/truncate/unflushed/target-name-recursive-no.log | Same as above but with recursive=no, verifying that this is also treated as unflushed. |
| test/fixtures/check-crash/query/truncate/unflushed/recursive-no.log | Unflushed truncate with io_flush?recursive=no (no target_name), ensuring recursive=no alone doesn’t mark truncate as flushed. |
| test/fixtures/check-crash/query/truncate/flushed/target-name-recursive-dependent.log | Flushed truncate with io_flush?target_name=Data&recursive=dependent, used to verify new recursive_dependent? branch clears truncate from unflushed stats. |
| test/fixtures/check-crash/query/truncate/flushed/recursive-yes.log | Flushed truncate with io_flush?recursive=yes (no target_name), exercising the existing flushed_objects-based branch. |
| test/fixtures/check-crash/query/table_create/unflushed/target-name-recursive-yes.log | Unflushed table_create plus io_flush?target_name=Data&recursive=yes, ensuring this combination doesn’t count as flushed. |
| test/fixtures/check-crash/query/table_create/unflushed/target-name-recursive-no.log | Unflushed table_create plus io_flush?target_name=Data&recursive=no, also expected to remain unflushed. |
| test/fixtures/check-crash/query/table_create/unflushed/recursive-no.log | Unflushed table_create with io_flush?recursive=no (no target_name), confirming no flush. |
| test/fixtures/check-crash/query/table_create/flushed/target-name-recursive-dependent.log | Flushed table_create plus io_flush?target_name=Data&recursive=dependent, testing new table_create handling in check_io_flush. |
| test/fixtures/check-crash/query/table_create/flushed/recursive-yes.log | Flushed table_create with io_flush?recursive=yes, validating the flushed_objects path for table creation. |
| test/fixtures/check-crash/query/load/unflushed/target-name-recursive-yes.log | Unflushed load with io_flush?target_name=Data&recursive=yes, asserting that non-dependent recursion with target_name is ignored for flushing. |
| test/fixtures/check-crash/query/load/unflushed/target-name-recursive-no.log | Unflushed load with io_flush?target_name=Data&recursive=no, also expected to remain unflushed. |
| test/fixtures/check-crash/query/load/unflushed/recursive-no.log | Unflushed load with io_flush?recursive=no (no target_name), confirming no flush occurs. |
| test/fixtures/check-crash/query/load/flushed/target-name-recursive-dependent.log | Flushed load with io_flush?target_name=Data&recursive=dependent, verifying that loads to Data are cleared as flushed. |
| test/fixtures/check-crash/query/load/flushed/recursive-yes.log | Flushed load with io_flush?recursive=yes, checking the existing recursive flush behavior without target_name. |
| test/fixtures/check-crash/query/delete/unflushed/target-name-recursive-yes.log | Unflushed delete with io_flush?target_name=Data&recursive=yes, asserting this does not mark delete as flushed. |
| test/fixtures/check-crash/query/delete/unflushed/target-name-recursive-no.log | Unflushed delete with io_flush?target_name=Data&recursive=no, also expected to remain unflushed. |
| test/fixtures/check-crash/query/delete/unflushed/recursive-no.log | Unflushed delete with io_flush?recursive=no, ensuring recursive=no alone doesn’t clear unflushed deletes. |
| test/fixtures/check-crash/query/delete/flushed/target-name-recursive-dependent.log | Flushed delete with io_flush?target_name=Data&recursive=dependent, exercising new delete handling in the recursive_dependent? branch. |
| test/fixtures/check-crash/query/delete/flushed/recursive-yes.log | Flushed delete with io_flush?recursive=yes, validating behavior without target_name. |
| test/fixtures/check-crash/query/column_create/unflushed/target-name-recursive-yes.log | Unflushed column_create with io_flush?target_name=Data.count&recursive=yes, used to confirm this does not flush the column. |
| test/fixtures/check-crash/query/column_create/unflushed/target-name-recursive-no.log | Unflushed column_create with io_flush?target_name=Data.count&recursive=no, also expected to remain unflushed. |
| test/fixtures/check-crash/query/column_create/unflushed/recursive-no.log | Unflushed column_create with io_flush?recursive=no (no target_name), verifying there is no flush. |
| test/fixtures/check-crash/query/column_create/flushed/target-name-recursive-dependent.log | Flushed column_create with io_flush?target_name=Data.count&recursive=dependent, testing that both Data.count (and related objects) are considered flushed. |
| test/fixtures/check-crash/query/column_create/flushed/recursive-yes.log | Flushed column_create with io_flush?recursive=yes, validating recursive flushing via flushed_objects for column creation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Related GitHub PR: groongaGH-87 https://groonga.org/docs/reference/commands/io_flush.html#usage According to the documentation, when using `io_flush` with `target_name`, it must be used together with `recursive=dependent`. Therefore, when used with `target_name`, only the `recursive=dependent` case will be flushed. Additionally, check for `target_name` in cases of `table_create` and `column_create` as well.
0a55b5e to
0fb3db6
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related GitHub PR: GH-87
https://groonga.org/docs/reference/commands/io_flush.html#usage
According to the documentation, when using
io_flushwithtarget_name, it must be used together withrecursive=dependent.Therefore, when used with
target_name, only therecursive=dependentcase will be flushed.Additionally, check for
target_namein cases oftable_createandcolumn_createas well.