Skip to content

crimson/osd/pg: do_osd_ops_execute refactor#54040

Merged
Matan-B merged 10 commits intoceph:mainfrom
Matan-B:wip-wip-matanb-crimson-do_osd_ops_execute-v3
Nov 1, 2023
Merged

crimson/osd/pg: do_osd_ops_execute refactor#54040
Matan-B merged 10 commits intoceph:mainfrom
Matan-B:wip-wip-matanb-crimson-do_osd_ops_execute-v3

Conversation

@Matan-B
Copy link
Contributor

@Matan-B Matan-B commented Oct 16, 2023

Preliminary refactor around do_osd_ops_execute() in preparation for: https://tracker.ceph.com/issues/61651.
Mainly for separating the two returned futures (submitted and all_completed).

Contribution Guidelines

Checklist

  • Tracker (select at least one)
    • References tracker ticket
    • Very recent bug; references commit where it was introduced
    • New feature (ticket optional)
    • Doc update (no ticket needed)
    • Code cleanup (no ticket needed)
  • Component impact
    • Affects Dashboard, opened tracker ticket
    • Affects Orchestrator, opened tracker ticket
    • No impact that needs to be tracked
  • Documentation (select at least one)
    • Updates relevant documentation
    • No doc update is appropriate
  • Tests (select at least one)
Show available Jenkins commands
  • jenkins retest this please
  • jenkins test classic perf
  • jenkins test crimson perf
  • jenkins test signed
  • jenkins test make check
  • jenkins test make check arm64
  • jenkins test submodules
  • jenkins test dashboard
  • jenkins test dashboard cephadm
  • jenkins test api
  • jenkins test docs
  • jenkins render docs
  • jenkins test ceph-volume all
  • jenkins test ceph-volume tox
  • jenkins test windows

@Matan-B Matan-B requested a review from a team as a code owner October 16, 2023 13:31
@Matan-B Matan-B force-pushed the wip-wip-matanb-crimson-do_osd_ops_execute-v3 branch from c588203 to 6f93b8e Compare October 18, 2023 07:37
add verbosity around returned futures, no change in behavior.

Signed-off-by: Matan Breizman <mbreizma@redhat.com>
be more explicit around returned futures, no change in behavior.

Signed-off-by: Matan Breizman <mbreizma@redhat.com>
Signed-off-by: Matan Breizman <mbreizma@redhat.com>
Signed-off-by: Matan Breizman <mbreizma@redhat.com>
Signed-off-by: Matan Breizman <mbreizma@redhat.com>
Signed-off-by: Matan Breizman <mbreizma@redhat.com>
@Matan-B Matan-B force-pushed the wip-wip-matanb-crimson-do_osd_ops_execute-v3 branch from 6f93b8e to 5001336 Compare October 19, 2023 12:13
Copy link
Contributor

@rzarzynski rzarzynski left a comment

Choose a reason for hiding this comment

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

Overall looks good. One question.

seastar::make_lw_shared<OpsExecuter>(
Ref<PG>{this}, obc, op_info, *m, conn, snapc),
m->ops,
// success_func
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Signed-off-by: Matan Breizman <mbreizma@redhat.com>
Signed-off-by: Matan Breizman <mbreizma@redhat.com>
@Matan-B Matan-B force-pushed the wip-wip-matanb-crimson-do_osd_ops_execute-v3 branch from 5001336 to dd5c18a Compare October 22, 2023 11:12
@Matan-B Matan-B requested a review from rzarzynski October 22, 2023 11:13
Signed-off-by: Matan Breizman <mbreizma@redhat.com>
Signed-off-by: Matan Breizman <mbreizma@redhat.com>
@Matan-B
Copy link
Contributor Author

Matan-B commented Oct 24, 2023

jenkins retest this please

@Matan-B
Copy link
Contributor Author

Matan-B commented Oct 31, 2023

@cyx1231st cyx1231st self-requested a review October 31, 2023 14:37
Copy link
Member

@cyx1231st cyx1231st left a comment

Choose a reason for hiding this comment

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

LGTM, looks the behavior is unchanged, so probably worth adding keyword "cleanup" to the PR title?

@Matan-B Matan-B merged commit 6906133 into ceph:main Nov 1, 2023
@Matan-B
Copy link
Contributor Author

Matan-B commented Nov 1, 2023

LGTM, looks the behavior is unchanged, so probably worth adding keyword "cleanup" to the PR title?

Yes, no behavior change. "Refactor" was added to the title, I'll add cleanup label as well. Thanks!

Merging based on tests.

failures:
7441958 - (osd.1) https://tracker.ceph.com/issues/61651
7441964 - (osd.2) https://tracker.ceph.com/issues/61651
7441986 - (osd.2) https://tracker.ceph.com/issues/61651
perf

@Matan-B Matan-B added the cleanup label Nov 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants