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

Remove old code for tracking operations #4842

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

AlekSi
Copy link
Member

@AlekSi AlekSi commented Feb 27, 2025

Description

Closes #4794.

Readiness checklist

  • I added/updated unit tests (and they pass).
  • I added/updated integration/compatibility tests (and they pass).
  • I added/updated comments and checked rendering.
  • I made spot refactorings.
  • I updated user documentation.
  • I ran task all, and it passed.
  • I ensured that PR title is good enough for the changelog.
  • (for maintainers only) I set Reviewers (@FerretDB/core), Milestone (Next), Labels, Project and project's Sprint fields.
  • I marked all done items in this checklist.

@AlekSi AlekSi added the code/chore Code maintenance improvements label Feb 27, 2025
@AlekSi AlekSi added this to the Next milestone Feb 27, 2025
@AlekSi AlekSi self-assigned this Feb 27, 2025
@AlekSi AlekSi requested a review from Copilot February 27, 2025 17:03

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")
@AlekSi AlekSi marked this pull request as ready for review February 27, 2025 17:20
@AlekSi AlekSi enabled auto-merge (squash) February 27, 2025 17:21
@AlekSi AlekSi requested review from a team, chilagrow, noisersup and Copilot February 27, 2025 17:21

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")
Copy link
Member

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.

Copy link
Member

@noisersup noisersup left a comment

Choose a reason for hiding this comment

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

CI fails

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code/chore Code maintenance improvements
Projects
Status: In progress
Development

Successfully merging this pull request may close these issues.

Reimplement currentOp to use DocumentDB
3 participants