Meeting Notes - 2017-10-05

[Sijie Guo <gu...@gmail.com>]

Hi all,

Here is the meeting notes :
https://cwiki.apache.org/confluence/display/BOOKKEEPER/2017-10-05+Meeting+Notes

Please point out if I missed anything we discussed at the meeting.

- Sijie

Re: Meeting Notes - 2017-10-05

[Ivan Kelly <iv...@apache.org>]

>> > The problem will arise when I need to address comments in the first
>> patches
>> > or I need to rebase to master
>> Why? You only need to do this once. Fixes can be merged into the
>> pertinent commit as a fixup when you do the rebase.
>>
>
> Thank you, let me play with git, I will learn how to handle this stuff
> better
ping me on slack if you want any pointers :)

-Ivan

Re: Meeting Notes - 2017-10-05

[Enrico Olivelli <eo...@gmail.com>]

2017-10-06 11:40 GMT+02:00 Ivan Kelly <iv...@apache.org>:

> > and this (first part)
> > https://github.com/apache/bookkeeper/pull/531
> This is the one I was looking at. There's only one commit, which I
> think could be broken up. Obviously, everyone will have a different
> opinion on where to make these cuts.
>

You are right, honestly I never did such cuts and I am not very good with
this workflow in git.
I am finishing the work for BP-15 (which is still a huge commit) and then I
will be back to that patch (BP-14)
I will play with git and learn how to do fixups


>
> > The problem will arise when I need to address comments in the first
> patches
> > or I need to rebase to master
> Why? You only need to do this once. Fixes can be merged into the
> pertinent commit as a fixup when you do the rebase.
>

Thank you, let me play with git, I will learn how to handle this stuff
better

Enrico


> -Ivan
>

Re: Meeting Notes - 2017-10-05

[Ivan Kelly <iv...@apache.org>]

> and this (first part)
> https://github.com/apache/bookkeeper/pull/531
This is the one I was looking at. There's only one commit, which I
think could be broken up. Obviously, everyone will have a different
opinion on where to make these cuts.

> The problem will arise when I need to address comments in the first patches
> or I need to rebase to master
Why? You only need to do this once. Fixes can be merged into the
pertinent commit as a fixup when you do the rebase.

-Ivan

Re: Meeting Notes - 2017-10-05

[Enrico Olivelli <eo...@gmail.com>]

2017-10-06 11:30 GMT+02:00 Ivan Kelly <iv...@apache.org>:

> > I think that the best we can do it to always split a commit to minimal
> > components, but actually we are working this way
> >
> > I am struggling with the BP-14 feature because I am trying to split the
> > work but without a good result, the patch impacts:
> > - client side API
> > - zk metadata
> > - wire protocol
> > - journal
> > - LAC protocol
> A lot of these pieces can be done independently though. You can put
> the metadata stuff a commit, and just have nothing use the new fields.
> You can implement the last add synced, without having the different
> sync types in the bookie or client. You can implement the sync types
> in the bookie, without supporting it in the client. You can do the
> client without changing the benchs.
>
> Obviously you'll probably to touching all over the code when you're
> developing the feature, but once it's ready to go, you can build up
> your commits. I usually commit a WIP commit at this point, reset back
> to master, and use "git add -p". After each commit, stash the
> uncommitted, make sure the branch compiles (and tests pass if there's
> something dodgy in the patch), rinse and repeat.
>


Yes
It is like I am trying to do with this (full patch)
https://github.com/apache/bookkeeper/pull/529

and this (first part)
https://github.com/apache/bookkeeper/pull/531

The problem will arise when I need to address comments in the first patches
or I need to rebase to master

Thank you for your suggestions


Apart from this "huge" new features I think that we are working well with
smal bugfixes


Enrico




>
> -Ivan
>

Re: Meeting Notes - 2017-10-05

[Ivan Kelly <iv...@apache.org>]

> I think that the best we can do it to always split a commit to minimal
> components, but actually we are working this way
>
> I am struggling with the BP-14 feature because I am trying to split the
> work but without a good result, the patch impacts:
> - client side API
> - zk metadata
> - wire protocol
> - journal
> - LAC protocol
A lot of these pieces can be done independently though. You can put
the metadata stuff a commit, and just have nothing use the new fields.
You can implement the last add synced, without having the different
sync types in the bookie or client. You can implement the sync types
in the bookie, without supporting it in the client. You can do the
client without changing the benchs.

Obviously you'll probably to touching all over the code when you're
developing the feature, but once it's ready to go, you can build up
your commits. I usually commit a WIP commit at this point, reset back
to master, and use "git add -p". After each commit, stash the
uncommitted, make sure the branch compiles (and tests pass if there's
something dodgy in the patch), rinse and repeat.

-Ivan

Re: Meeting Notes - 2017-10-05

[Enrico Olivelli <eo...@gmail.com>]

2017-10-06 10:25 GMT+02:00 Ivan Kelly <iv...@apache.org>:

> > I can't see this problem with the actual merge script, it works very
> well and on the git history we will have only one "commit" for issue.
> The issue is that this "one commit" is very difficult to work with.
>
> My understanding was that Salesforce have their own branch, with
> private changes. Each time we release, they want to merge the new
> changes into their private branch. I've had to deal with this kind of
> relationship between branches and repos before, and in these
> scenarios, small self contained commits that only do one thing are
> much better to work with. You have fewer conflicts and when conflicts
> do occur its clearer what should be done with the conflict.
>
> While this may not be a concern for the project now, as we're really
> only working on master, as adoption grows we could easily see a
> situation where there's a super stable branch, which we only pull back
> bugfixes to. Small commits makes this easier. It also facilitates
> selecting which bugfixes to pull back by looking at the commit log
> rather than by looking at github/jira (which may not be up to date).
> In general it makes the commit log much more useful. If you need to
> bisect a branch to find when a bug was introduced, small commits let
> you pinpoint the offending change more accurately.
>
> Finally, it gives the committer a chance to curate their changes,
> allowing them to clarify their intent, not only to reviewers, but also
> to themselves.
>
> In short, I don't like big commits :)
>


I don't like them too.
Sometimes it is difficult to split a commit in multiple changes.
The "local" git history for an issue is not usually good to be copied to
upstream (Apache master) because it would be full of local minor fixes.

I think that the best we can do it to always split a commit to minimal
components, but actually we are working this way

I am struggling with the BP-14 feature because I am trying to split the
work but without a good result, the patch impacts:
- client side API
- zk metadata
- wire protocol
- journal
- LAC protocol

it is difficult to have a single commit which makes sense on its own, but
it is useful for reviews to have single commits
but at the end of the story I image it will be pushed as one or two commits



Enrico




>
> -Ivan
>

Re: Meeting Notes - 2017-10-05

[Ivan Kelly <iv...@apache.org>]

> I can't see this problem with the actual merge script, it works very well and on the git history we will have only one "commit" for issue.
The issue is that this "one commit" is very difficult to work with.

My understanding was that Salesforce have their own branch, with
private changes. Each time we release, they want to merge the new
changes into their private branch. I've had to deal with this kind of
relationship between branches and repos before, and in these
scenarios, small self contained commits that only do one thing are
much better to work with. You have fewer conflicts and when conflicts
do occur its clearer what should be done with the conflict.

While this may not be a concern for the project now, as we're really
only working on master, as adoption grows we could easily see a
situation where there's a super stable branch, which we only pull back
bugfixes to. Small commits makes this easier. It also facilitates
selecting which bugfixes to pull back by looking at the commit log
rather than by looking at github/jira (which may not be up to date).
In general it makes the commit log much more useful. If you need to
bisect a branch to find when a bug was introduced, small commits let
you pinpoint the offending change more accurately.

Finally, it gives the committer a chance to curate their changes,
allowing them to clarify their intent, not only to reviewers, but also
to themselves.

In short, I don't like big commits :)

-Ivan

Re: Meeting Notes - 2017-10-05

[Enrico Olivelli - Diennea <en...@diennea.com>]

Thank you

Just a note (maybe I will start a new thread)
but actually the merge scripts does merge --squash and the effect is the same as doing a rebase to master

I can't see this problem with the actual merge script, it works very well and on the git history we will have only one "commit" for issue.
Usually during the discussion on GitHub it is good not to rebase and do force pushes in local branchs, this way the reviewer can see how its comments gets addressed.
The GitHub UI is very powerful in aggregating the commits of the PR
Sometimes I see it is better to do a "git rebase -i " and squash locally the commits before updating the PR

Enrico

Il giorno ven, 06/10/2017 alle 09.47 +0800, Jia Zhai ha scritto:

👍

On Fri, Oct 6, 2017 at 4:42 AM, Sijie Guo <gu...@gmail.com>> wrote:



Hi all,

Here is the meeting notes :
https://cwiki.apache.org/confluence/display/BOOKKEEPER/
2017-10-05+Meeting+Notes

Please point out if I missed anything we discussed at the meeting.

- Sijie



--

Enrico Olivelli Software Development Manager @Diennea Tel.: (+39) 0546 066100 - Int. 925 Viale G.Marconi 30/14 - 48018 Faenza (RA) MagNews - E-mail Marketing Solutions http://www.magnews.it Diennea - Digital Marketing Solutions http://www.diennea.com

________________________________

Iscriviti alla nostra newsletter per rimanere aggiornato su digital ed email marketing! http://www.magnews.it/newsletter/

The information in this email is confidential and may be legally privileged. If you are not the intended recipient please notify the sender immediately and destroy this email. Any unauthorized, direct or indirect, disclosure, copying, storage, distribution or other use is strictly forbidden.

Re: Meeting Notes - 2017-10-05

[Jia Zhai <zh...@gmail.com>]

👍

On Fri, Oct 6, 2017 at 4:42 AM, Sijie Guo <gu...@gmail.com> wrote:

> Hi all,
>
> Here is the meeting notes :
> https://cwiki.apache.org/confluence/display/BOOKKEEPER/
> 2017-10-05+Meeting+Notes
>
> Please point out if I missed anything we discussed at the meeting.
>
> - Sijie
>