Re: [SPDK] histogram patches

* Re: [SPDK] histogram patches
@ 2017-11-27 20:52 Meneghini, John
  0 siblings, 0 replies; 5+ messages in thread
From: Meneghini, John @ 2017-11-27 20:52 UTC (permalink / raw)
  To: spdk

[-- Attachment #1: Type: text/plain, Size: 5890 bytes --]

Hi Jim.

Yes, I tried to update the old pull request.  GerritHub wouldn’t cooperate. I opened a new pull request because, after multiple attempts to update the old one, I could not get GerritHub accept my update.

I know my local repository is working correctly because it generated a new Change-Id after I abandon the old pull request and created a new one.  Also, I had a lot of problems getting this old patch set to rebase. I eventually just did a “git reset –soft” and re-implemented everything from scratch.
/John

johnm-mac-0:spdk(remote_histogram_changes24) > git push origin HEAD:refs/for/master
Counting objects: 53, done.
Delta compression using up to 8 threads.
Compressing objects: 100% (52/52), done.
Writing objects: 100% (53/53), 15.17 KiB | 3.03 MiB/s, done.
Total 53 (delta 39), reused 0 (delta 0)
remote: Resolving deltas: 100% (39/39)
remote: Processing changes: refs: 1, done
To https://review.gerrithub.io/spdk/spdk
! [remote rejected]   HEAD -> refs/for/master (duplicate request)
error: failed to push some refs to 'https://review.gerrithub.io/spdk/spdk'
johnm-mac-0:spdk(remote_histogram_changes24) > git log -1
commit b2ff67c24347d87948a746c20e6dcc3b8a7a0140 (HEAD -> remote_histogram_changes24)
Author: Girish Chandrashekar <Girish.Chandrashekar(a)netapp.com>
Date:   Thu Nov 23 00:03:34 2017 -0500

    test: add unit tests for histogram facility

    Change-Id: I6db92dda23ed8cabef7f222aba0c9523e8f75bf7
    Signed-off-by: John Meneghini <johnm(a)netapp.com>
    Signed-off-by: Paul Luse <paul.e.luse(a)intel.com>

From: SPDK <spdk-bounces(a)lists.01.org> on behalf of Jim Harris <james.r.harris(a)intel.com>
Reply-To: Storage Performance Development Kit <spdk(a)lists.01.org>
Date: Monday, November 27, 2017 at 12:19 PM
To: Storage Performance Development Kit <spdk(a)lists.01.org>
Cc: "Chandrashekar, Girish" <Girish.Chandrashekar(a)netapp.com>
Subject: Re: [SPDK] histogram patches

Hi John,

Thanks for pushing out the new request.  Could you use the original Gerrit commit ID instead of starting a new one?  This will allow us to keep the history from the earlier reviews, to make sure all of the previous comments were addressed.

You should just need to:

  1.  Restore https://review.gerrithub.io/#/c/363114/
  2.  Modify the Change-Id in your new patch to Change-Id: I6db92dda23ed8cabef7f222aba0c9523e8f75bf7 (this is the same as the original patch Girish submitted)

-Jim

From: SPDK <spdk-bounces(a)lists.01.org> on behalf of "Meneghini, John" <John.Meneghini(a)netapp.com>
Reply-To: Storage Performance Development Kit <spdk(a)lists.01.org>
Date: Sunday, November 26, 2017 at 8:57 PM
To: Storage Performance Development Kit <spdk(a)lists.01.org>
Cc: "Chandrashekar, Girish" <Girish.Chandrashekar(a)netapp.com>
Subject: Re: [SPDK] histogram patches

As agreed during the SPDK Meetup early this month, Girish and I have rebased and updated the Histogram Utility changes and published a new pull request.

https://review.gerrithub.io/#/c/389053/

Thanks to Paul Luse for his help and encouragement in getting this done.

--
John Meneghini
johnm(a)netapp.com

From: "Chandrashekar, Girish" <Girish.Chandrashekar(a)netapp.com>
Date: Tuesday, November 14, 2017 at 1:00 PM
To: John Meneghini <John.Meneghini(a)netapp.com>, "Luse, Paul E" <paul.e.luse(a)intel.com>
Subject: Re: histogram patches

Hi John,
   Changes to rdma.c will not affect much as it comes under #ifdef HISTOGRAM,
spdk/lib/env_dpdk/pci_virtio.c can be ignored as it has empty line as diff

Paul has to wait, since I missed to address 14 minor comments from Daniel. I will finish this before weekend along with unit test.
Can I use remote_histogram_changes22 branch to address these comments?

Regards
Girish

From: Girish Chandrashekar <Girish.Chandrashekar(a)netapp.com>
Date: Monday, November 13, 2017 at 10:45 AM
To: "Meneghini, John" <John.Meneghini(a)netapp.com>, "Luse, Paul E" <paul.e.luse(a)intel.com>
Subject: Re: histogram patches

Thanks John, I will test once patch is ready

Regards
Girish

From: "Meneghini, John" <John.Meneghini(a)netapp.com>
Date: Monday, November 13, 2017 at 10:44 AM
To: "Luse, Paul E" <paul.e.luse(a)intel.com>, Girish Chandrashekar <Girish.Chandrashekar(a)netapp.com>
Cc: "Meneghini, John" <John.Meneghini(a)netapp.com>
Subject: Re: histogram patches

Sure thing Paul.  Thanks for your help.

Girish, I’ll download these patches on Monday and pull them into the VED so you can test them out.  In the meantime, you can see Paul’s updates to the pull request in GerritHub at:

https://review.gerrithub.io/#/c/363114/

If everything looks good, please feel free to give Paul the go ahead and these patches can be checked into SPDK.

I’ve already approved these changes.

/John

From: "Luse, Paul E" <paul.e.luse(a)intel.com>
Date: Saturday, November 11, 2017 at 1:55 PM
To: John Meneghini <John.Meneghini(a)netapp.com>
Subject: histogram patches

Hi John,

I rebased the patches last week so they’re ready for Girish to take a look at, fix where I may have missed something and address Daniel’s last set of comments.  Two things during the rebase:

- in rdma.c there were two ‘finish markers’ that I couldn’t find a good home for in the refactored code.  I think I got one of them right but either Girish or Ben or someone will have to pinpoint where the other marker should go, I just have never looked at any of the NVMeoF stuff before so was essentially guessing
- the rebase picked up some conflicts in spdk/lib/env_dpdk/pci_virtio.c which seemed odd since it wasn’t touced in any of the patches that I could tell so I just took the latest so he might want to double check and make sure there’s not something missing there

Other than that it was pretty straightforward

Thx
Paul

[-- Attachment #2: attachment.html --]
[-- Type: text/html, Size: 18053 bytes --]

^ permalink raw reply	[flat|nested] 5+ messages in thread