-
Notifications
You must be signed in to change notification settings - Fork 417
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
Remove old code for tracking operations #4842
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Overview
This PR removes the obsolete operation tracking code and its usage throughout various handler and integration test files for FerretDB.
- Removed operation tracking functions and registry updates in the handler messages.
- Updated integration tests to stop referencing operation tracking.
- Deleted the entire operation package files.
Reviewed Changes
File | Description |
---|---|
integration/currentop_test.go | Updated test function parameter naming and wrapper call. |
internal/handler/handler.go | Removed operations registry field and its cleanup call. |
internal/handler/msg_currentop.go | Removed operation tracking and replaced op details with an empty array. |
internal/handler/msg_explain.go, msg_findandmodify.go, etc. | Removed operation start/stop and update calls in various handler messages. |
internal/handler/operation/{operation.go, registry.go} | Removed obsolete operation tracking package. |
Copilot reviewed 15 out of 15 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (2)
integration/currentop_test.go:39
- [nitpick] The local variable 't' shadows the test parameter 'tt'; consider renaming it (e.g., 'subT' or 'testT') to avoid confusion.
t := setup.FailsForFerretDB(tt, "https://github.com/FerretDB/FerretDB/issues/4794")
integration/currentop_test.go:173
- [nitpick] Similar to the other test case, the variable 't' shadows 'tt'; renaming it would enhance clarity.
t := setup.FailsForFerretDB(tt, "https://github.com/FerretDB/FerretDB/issues/4794")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Overview
This PR removes legacy operation tracking code from the codebase to simplify request handling. Key changes include:
- Removal of operation tracking logic and update calls in message handlers.
- Cleanup of operations registry and related fields from the handler struct.
- Updates to test functions to reflect the removal of operation tracking.
Reviewed Changes
File | Description |
---|---|
integration/currentop_test.go | Renames test parameters and updates test setup related to currentOp. |
internal/handler/msg_explain.go | Removes obsolete operation tracking calls from the explain command. |
internal/handler/handler.go | Eliminates operations registry usage from handler creation and shutdown. |
internal/handler/msg_currentop.go (and similar files) | Removes tracking operations code from various message handlers. |
internal/handler/operation/operation.go | Deletes legacy operation structure and related code. |
internal/handler/operation/registry.go | Deletes legacy registry implementation. |
Copilot reviewed 15 out of 15 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (1)
integration/currentop_test.go:39
- [nitpick] The re-assignment of the test variable 't' in a function with parameter 'tt' could cause confusion. Consider renaming the local 't' to a more descriptive name such as 'testCtx' to improve clarity.
t := setup.FailsForFerretDB(tt, "https://github.com/FerretDB/FerretDB/issues/4794")
tt.Parallel() | ||
|
||
// TODO https://github.com/FerretDB/FerretDB/issues/4794 | ||
t := setup.FailsForFerretDB(tt, "https://github.com/FerretDB/FerretDB/issues/4794") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't tell why other tests (not this test) are failing on CI. It looks like something we've seen before, context getting cancelled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CI fails
Description
Closes #4794.
Readiness checklist
task all
, and it passed.@FerretDB/core
), Milestone (Next
), Labels, Project and project's Sprint fields.