2024-06-20T06:01:34.196Z | <Rishabh Dave> @Venky Shankar Thanks for review! The last commit (linked above), looks fine? Can I squash it? |
2024-06-20T07:22:20.659Z | <Rishabh Dave> ping |
2024-06-20T07:22:50.359Z | <Venky Shankar> I left a comment there |
2024-06-20T07:23:04.672Z | <Rishabh Dave> Didn't see in my notifications, thanks! |
2024-06-20T07:23:34.114Z | <Venky Shankar> the commit is gone |
2024-06-20T07:23:34.684Z | <Venky Shankar> lol |
2024-06-20T07:23:46.481Z | <Rishabh Dave> the link is broken, searching for it. |
2024-06-20T07:24:22.027Z | <Rishabh Dave> In the mean time please take a look at this - <https://github.com/ceph/ceph/pull/54620#discussion_r1647078541> |
2024-06-20T07:26:27.933Z | <Venky Shankar> oh |
2024-06-20T07:26:33.030Z | <Venky Shankar> comments are in pending state |
2024-06-20T07:26:34.177Z | <Venky Shankar> 😕 |
2024-06-20T07:28:24.278Z | <Rishabh Dave> okay. lots of comments, were posted much earlier compared rest of bulk of review. if you could post the earlier immediately, i would start working on it. |
2024-06-20T07:36:07.935Z | <Rishabh Dave> Venky, can you please use `Add a comment` button instead of `Start review`? When using latter button it beomes really tough to find original thread because GitHub posts the reply separately. But if you use former GitHub directly takes me to the original thread.
Especially because there plenty comment and GitHub hides all of them it becomes tough to find those threads.: https://files.slack.com/files-pri/T1HG3J90S-F078Y89R38T/download/image.png |
2024-06-20T07:36:07.938Z | <Rishabh Dave> https://files.slack.com/files-pri/T1HG3J90S-F079B1WN789/download/image.png |
2024-06-20T07:38:22.189Z | <Venky Shankar> The problem with that is I might be half way reviewing and then if I want to reply to a comment, it automatically adds as pending. |
2024-06-20T07:43:25.997Z | <Rishabh Dave> Oh, okay. I see. Perhaps you can use `Add a comment` instead of `Start review` at least when replying to a single comment. |
2024-06-20T07:51:56.033Z | <Rishabh Dave> > I left a comment there
Can't find it. Is it pending by any chance? |
2024-06-20T07:56:23.986Z | <Venky Shankar> forget it - I left the comment again |
2024-06-20T08:06:59.901Z | <Rishabh Dave> Cool, thanks! |
2024-06-20T09:06:42.426Z | <Rishabh Dave> @Venky Shankar This quincy backport PR (<https://github.com/ceph/ceph/pull/58087>) has 1 commit but for this we'll need to backport another ticket that hasn't been backported beyond reef. This ticket itself is linked with 2 PRs, so we'll need to backport 4 more commits, maybe even more. Should I proceed find all relevant commits and backport each of them to Quincy or drop PR 58087 since Quincy soon won't be very active soon? |
2024-06-20T09:18:18.935Z | <Venky Shankar> If its getting to pulling in commits from multiple PRs, then we can not do the backport for quincy since its not a urgent fix. |
2024-06-20T09:22:03.071Z | <Rishabh Dave> okay. |
2024-06-20T09:23:51.499Z | <Rishabh Dave> Equivalent downstream version will be supported for much more time, right? in that case perhaps (i am not sure) to backport all these commits since a python module used cephfs-shell has been deprecated. But I don't if that's a good reason to go through all this extra effort. |
2024-06-20T09:24:50.805Z | <Venky Shankar> I don't think we were packaging it downstream until lately. |
2024-06-20T09:28:37.966Z | <Leonid Usov> I think that to make the backporting easier we should include all relevant tickets in a single PR |
2024-06-20T09:29:40.478Z | <Venky Shankar> that's true, but at times its becomes really messy and then we really to decide if the backport effort is really worth it esp. if a release is going to be EOL'd soon |
2024-06-20T09:30:48.557Z | <Leonid Usov> The PTL script is great but it can’t deal with the complex relationship between PRs. It may fail to cherry-pick them |
2024-06-20T09:33:24.821Z | <Rishabh Dave> > I think that to make the backporting easier we should include all relevant tickets in a single PR
agreed but few months ago i spent a lot of time since a 9 commit PR needed backport of 32 commits. worse, i had to abandon it because it required even more commits. |
2024-06-20T09:48:05.826Z | <Jos Collin> I think you have to cherry-pick that LooseVersion fix too and update the PR 58087. Otherwise, it won't pass the make check. |
2024-06-20T09:48:35.332Z | <Rishabh Dave> Yeah, I went through that ticket and associated PRs. |
2024-06-20T09:49:05.663Z | <Jos Collin> If it doesn't pass make check, we can't do qa. |
2024-06-20T09:49:17.873Z | <Jos Collin> for that pr |
2024-06-20T09:49:37.723Z | <Rishabh Dave> Yes, IIUC, Venky is fine with dropping this backport. |
2024-06-20T09:49:55.277Z | <Jos Collin> okay fine then. |
2024-06-20T09:50:15.219Z | <Rishabh Dave> @Venky Shankar Can I get a confirmation? I proceed to close that backport PR... |
2024-06-20T09:51:55.289Z | <Venky Shankar> how many commits are we talking about and how large are those? |
2024-06-20T09:52:14.244Z | <Rishabh Dave> checking... |
2024-06-20T09:54:14.436Z | <Rishabh Dave> at least 4 more commits. ~40 LoC. There might be more commits, that I can't find without cherry-picking and letting `make check` finish. |
2024-06-20T09:54:26.974Z | <Rishabh Dave> at least 4 more commits. ~40 LoC. There might be more commits, that I can't find that without cherry-picking and letting `make check` finish. |
2024-06-20T09:55:46.891Z | <Venky Shankar> if its only for quincy, let's drop the backport. |
2024-06-20T09:57:05.773Z | <Rishabh Dave> Yes, only for Quincy. Dropping. |
2024-06-20T10:45:30.362Z | <Rishabh Dave> @Venky Shankar is it okay to move tests from `clone stats` PR outside `test_volumes.py`? what about `qa/tasks/cephfs/volumes/test_vol_clone_progress` ? |
2024-06-20T10:45:57.001Z | <Venky Shankar> let's _not_ do that now |
2024-06-20T10:54:20.121Z | <Rishabh Dave> @Venky Shankar please reply on this thread - <https://github.com/ceph/ceph/pull/54620#discussion_r1647083027> |
2024-06-20T11:05:39.196Z | <Rishabh Dave> Thank you! |
2024-06-20T11:13:49.338Z | <Rishabh Dave> @Venky Shankar Let's merge this method (<https://github.com/ceph/ceph/pull/54620/files#diff-c0d245e49ccb876cd07bfddafb303a1b366b8a2c70f1e16c9bfbd4f8c79cf11dR271-R285>)
into this one - <https://github.com/ceph/ceph/pull/54620/files#diff-c0d245e49ccb876cd07bfddafb303a1b366b8a2c70f1e16c9bfbd4f8c79cf11dR218-R269> ? |
2024-06-20T11:26:50.824Z | <Venky Shankar> maybe, yes |
2024-06-20T11:30:54.484Z | <Rishabh Dave> okay, done - <https://github.com/ceph/ceph/compare/5747d46b0098448425194ab9a06ab8ce9d1deee4..a8f98bde8df011e54efaeeea904366aa2c98b006>. |