Draft Agenda for Bi-weekly meeting at 2017-07-27

[Jia Zhai]

Hi all, :)
I would like to put a draft Agenda here

for
7/27's meeting, this is just limited from my point of view, Please feel
free to add anything you want to discuss in this meeting.

*Agenda:*

   - Status of 4.5 release
   - New BookKeeper Website
   - Slack channel
   - 4.5 Performance regression?
   - Status of Bookkeeper Docker image
   - Other issues and PRs

Thanks.

Re: BP-11: New BookKeeper Website

[Jia Zhai]

Awesome

On Wed, Jul 26, 2017 at 6:04 AM, Ivan Kelly  wrote:

> Looks great :D. This has been long overdue.
>
> My one piece of feedback is that perhaps the font is too big on the
> documentation. It means that there isn't much content on the page
> before having to scroll (See attached screenshot). My resolution is
> 1360x768.
>
> -Ivan
>
>
> On Tue, Jul 25, 2017 at 9:38 PM, Venkateswara Rao Jujjuri
>  wrote:
> > Awesome !! lets make it dense. :)
> >
> > On Tue, Jul 25, 2017 at 12:15 PM, Yiming Zang  >
> > wrote:
> >
> >> This looks awesome!
> >>
> >> On Tue, Jul 25, 2017 at 12:09 PM, Sijie Guo  wrote:
> >>
> >> > Hi all,
> >> >
> >> > Luc Perkins is helping on a new bookkeeper documentation site for
> BP-11.
> >> > Although it is still WIP, we'd like to first share the beta version
> with
> >> > the community to get some feedbacks.
> >> >
> >> > The new website: https://lucperkins.github.io/bookkeeper/
> >> > The pull request (still WIP) : https://github.com/apache/
> >> > bookkeeper/pull/293
> >> >
> >> > - Sijie
> >> >
> >>
> >
> >
> >
> > --
> > Jvrao
> > ---
> > First they ignore you, then they laugh at you, then they fight you, then
> > you win. - Mahatma Gandhi
>

Re: [DISCUSS] Slack Channel for BookKeeper

[Sijie Guo]

Currently BK doesn't has one yet.

On Tue, Jul 25, 2017 at 9:29 AM, Henry Saputra 
wrote:

> Is Apache BookKeeper has its own Slack team? If it has then we could just
> add a channel for DistLog.
>
> - Henry
>
> On Mon, Jul 24, 2017 at 11:42 PM, Enrico Olivelli 
> wrote:
>
> > Awesome +1
> >
> > Il mar 25 lug 2017, 05:39 Dustin Castor 
> > ha
> > scritto:
> >
> > > Agreed! Or an IRC.
> > >
> > > On Monday, July 24, 2017, 6:50:22 PM PDT, Jia Zhai <
> zhaiji...@gmail.com>
> > > wrote:
> > >
> > >  It is great to have a slack channel. It make things more effective
> and
> > > smooth.
> > >
> > > On Tue, Jul 25, 2017 at 8:11 AM, Sijie Guo  wrote:
> > >
> > > > Hi all,
> > > >
> > > > What do you guys all think about having a dedicated slack channel for
> > > > informal discussion for the community? There are a handful of Apache
> > > > projects are doing that already, there are also ways to have a bot
> that
> > > > sends daily digest of the conversation to the mailing lists (to keep
> > the
> > > > records in ASF infrastructure).
> > > >
> > > > As the followup steps for merging DL into BookKeeper, we are
> > transferring
> > > > the DL slack channel to BookKeeper PMC. We can just make it a BK
> slack
> > > > channel, and have different channels under it for different
> > discussions.
> > > >
> > > > Thoughts?
> > > >
> > > > - Sijie
> > > >
> >
> > --
> >
> >
> > -- Enrico Olivelli
> >
>

[GitHub] zhaijack opened a new pull request #298: Issue 295: LocalBookKeeper fails to start up

[git]

zhaijack opened a new pull request #298: Issue 295: LocalBookKeeper fails to 
start up
URL: https://github.com/apache/bookkeeper/pull/298
 
 
   Descriptions of the changes in this PR:
   
   remote the zk port and zk servers from the classes and use the values passed 
in from the method.
 

This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] zhaijack commented on issue #297: Issue 296: Bookie supports ephemeral port

[git]

zhaijack commented on issue #297: Issue 296: Bookie supports ephemeral port
URL: https://github.com/apache/bookkeeper/pull/297#issuecomment-317907200
 
 
   ported the change for @sijie 
 

This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] zhaijack opened a new pull request #297: Issue 296: Bookie supports ephemeral port

[git]

zhaijack opened a new pull request #297: Issue 296: Bookie supports ephemeral 
port
URL: https://github.com/apache/bookkeeper/pull/297
 
 
   Descriptions of the changes in this PR:
   
   - add a flag to allow/disable using ephemeral ports
   - change the initialization sequence to support ephmeral port
   - added two tests on verifying this behavior
   
 

This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

Re: BP-11: New BookKeeper Website

[Ivan Kelly]

Looks great :D. This has been long overdue.

My one piece of feedback is that perhaps the font is too big on the
documentation. It means that there isn't much content on the page
before having to scroll (See attached screenshot). My resolution is
1360x768.

-Ivan


On Tue, Jul 25, 2017 at 9:38 PM, Venkateswara Rao Jujjuri
 wrote:
> Awesome !! lets make it dense. :)
>
> On Tue, Jul 25, 2017 at 12:15 PM, Yiming Zang 
> wrote:
>
>> This looks awesome!
>>
>> On Tue, Jul 25, 2017 at 12:09 PM, Sijie Guo  wrote:
>>
>> > Hi all,
>> >
>> > Luc Perkins is helping on a new bookkeeper documentation site for BP-11.
>> > Although it is still WIP, we'd like to first share the beta version with
>> > the community to get some feedbacks.
>> >
>> > The new website: https://lucperkins.github.io/bookkeeper/
>> > The pull request (still WIP) : https://github.com/apache/
>> > bookkeeper/pull/293
>> >
>> > - Sijie
>> >
>>
>
>
>
> --
> Jvrao
> ---
> First they ignore you, then they laugh at you, then they fight you, then
> you win. - Mahatma Gandhi

Re: [DISCUSS] Slack Channel for BookKeeper

[Matteo Merli]

On Tue, Jul 25, 2017 at 2:57 PM, Ivan Kelly  wrote:

> Is there an apache "team" in slack? it would save having to have
> multiple teams open if a person is involved in multiple projects. This
> would be especially useful for people who use slack in the browser. It
> would also reduce the signup barrier.


Yes, there's one: https://theasf.slack.com

Though it's only open to ASF committers because it requires a @apache.org
account.

Re: [DISCUSS] Slack Channel for BookKeeper

[Ivan Kelly]

Is there an apache "team" in slack? it would save having to have
multiple teams open if a person is involved in multiple projects. This
would be especially useful for people who use slack in the browser. It
would also reduce the signup barrier.

-Ivan

On Tue, Jul 25, 2017 at 6:29 PM, Henry Saputra  wrote:
> Is Apache BookKeeper has its own Slack team? If it has then we could just
> add a channel for DistLog.
>
> - Henry
>
> On Mon, Jul 24, 2017 at 11:42 PM, Enrico Olivelli 
> wrote:
>
>> Awesome +1
>>
>> Il mar 25 lug 2017, 05:39 Dustin Castor 
>> ha
>> scritto:
>>
>> > Agreed! Or an IRC.
>> >
>> > On Monday, July 24, 2017, 6:50:22 PM PDT, Jia Zhai 
>> > wrote:
>> >
>> >  It is great to have a slack channel. It make things more effective and
>> > smooth.
>> >
>> > On Tue, Jul 25, 2017 at 8:11 AM, Sijie Guo  wrote:
>> >
>> > > Hi all,
>> > >
>> > > What do you guys all think about having a dedicated slack channel for
>> > > informal discussion for the community? There are a handful of Apache
>> > > projects are doing that already, there are also ways to have a bot that
>> > > sends daily digest of the conversation to the mailing lists (to keep
>> the
>> > > records in ASF infrastructure).
>> > >
>> > > As the followup steps for merging DL into BookKeeper, we are
>> transferring
>> > > the DL slack channel to BookKeeper PMC. We can just make it a BK slack
>> > > channel, and have different channels under it for different
>> discussions.
>> > >
>> > > Thoughts?
>> > >
>> > > - Sijie
>> > >
>>
>> --
>>
>>
>> -- Enrico Olivelli
>>

[GitHub] zhaijack opened a new issue #296: Bookies supports ephemeral port

[git]

zhaijack opened a new issue #296: Bookies supports ephemeral port 
URL: https://github.com/apache/bookkeeper/issues/296
 
 
   **FEATURE REQUEST**
   
   1. Please describe the feature you are requesting.
   
   // If the caller specified ephemeral ports then use ephemeral 
ports for all
   // the bookies else use numBookie ports starting at initialPort
   if (0 == initialPort) {
   bsConfs[i].setBookiePort(0);
   } else {
   bsConfs[i].setBookiePort(initialPort + i);
   }
   
   Currently there is a logic in LocalBookKeeper to support ephemeral ports for 
testing, however the corresponding logic in Bookie is missing. Somehow DL uses 
this feature for testing, it would be nice to port the ephemeral port supports.
   
   2. Indicate the importance of this issue to you (blocker, must-have, 
should-have, nice-to-have). Are you currently using any workarounds to address 
this issue?
   
   **nice-to-have**. marked it as **nice-to-have**, but it might be **blocker** 
for DL to use latest BK. 
   
   3. Provide any additional detail on your proposed use case for this feature.
   
 

This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] zhaijack commented on issue #295: LocalBookKeeper failed to start

[git]

zhaijack commented on issue #295: LocalBookKeeper failed to start
URL: https://github.com/apache/bookkeeper/issues/295#issuecomment-317873950
 
 
   this should be a **blocker** for 4.5.0
 

This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] zhaijack opened a new issue #295: LocalBookKeeper failed to start

[git]

zhaijack opened a new issue #295: LocalBookKeeper failed to start
URL: https://github.com/apache/bookkeeper/issues/295
 
 
   **BUG REPORT**
   
   1. Please describe the issue you observed:
   
   - What did you do?
   
   Upgrade BK version to 4.5.0 in DL and run the tests.
   
   - What did you expect to see?
   
   The DL tests should pass.
   
   - What did you see instead?
   
   The DL tests fails because LocalBookKeeper can't startup.
   
   The cause is in following code: it doesn't set the correct zookeeper port.
   
   private void initializeZookeeper(int zkPort) throws IOException {
   LOG.info("Instantiate ZK Client");
   //initialize the zk client with values
   ZooKeeperClient zkc = null;
   try {
   zkc = ZooKeeperClient.newBuilder()
   
.connectString(InetAddress.getLoopbackAddress().getHostAddress() + ":" + 
zkDefaultPort)
   .sessionTimeoutMs(zkSessionTimeOut)
   .build();
   zkc.create("/ledgers", new byte[0], Ids.OPEN_ACL_UNSAFE, 
CreateMode.PERSISTENT);
   zkc.create("/ledgers/available", new byte[0], 
Ids.OPEN_ACL_UNSAFE, CreateMode.PERSISTENT);
   // No need to create an entry for each requested bookie anymore 
as the
   // BookieServers will register themselves with ZooKeeper on 
startup.
   } catch (KeeperException e) {
   LOG.error("Exception while creating znodes", e);
   throw new IOException("Error creating znodes : ", e);
   } catch (InterruptedException e) {
   LOG.error("Interrupted while creating znodes", e);
   throw new IOException("Error creating znodes : ", e);
   }
   }
 

This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

Re: BP-11: New BookKeeper Website

[Venkateswara Rao Jujjuri]

Awesome !! lets make it dense. :)

On Tue, Jul 25, 2017 at 12:15 PM, Yiming Zang 
wrote:

> This looks awesome!
>
> On Tue, Jul 25, 2017 at 12:09 PM, Sijie Guo  wrote:
>
> > Hi all,
> >
> > Luc Perkins is helping on a new bookkeeper documentation site for BP-11.
> > Although it is still WIP, we'd like to first share the beta version with
> > the community to get some feedbacks.
> >
> > The new website: https://lucperkins.github.io/bookkeeper/
> > The pull request (still WIP) : https://github.com/apache/
> > bookkeeper/pull/293
> >
> > - Sijie
> >
>



-- 
Jvrao
---
First they ignore you, then they laugh at you, then they fight you, then
you win. - Mahatma Gandhi

Re: BP-11: New BookKeeper Website

[Yiming Zang]

This looks awesome!

On Tue, Jul 25, 2017 at 12:09 PM, Sijie Guo  wrote:

> Hi all,
>
> Luc Perkins is helping on a new bookkeeper documentation site for BP-11.
> Although it is still WIP, we'd like to first share the beta version with
> the community to get some feedbacks.
>
> The new website: https://lucperkins.github.io/bookkeeper/
> The pull request (still WIP) : https://github.com/apache/
> bookkeeper/pull/293
>
> - Sijie
>

[GitHub] yzang commented on issue #266: Issue 265: Add persistable bookie status

[git]

yzang commented on issue #266: Issue 265: Add persistable bookie status
URL: https://github.com/apache/bookkeeper/pull/266#issuecomment-317842271
 
 
   @sijie That make sense, I understand what you mean, but my concern is:
   
   Even if we persist the bookie status on disk, when we set the bookie to read 
only through http endpoint, the bookie will still be transitioned back to 
writable very quickly, because the DiskChecker runs periodically and would do 
the transition on each check, so there's no point setting the bookie to 
readonly on the fly, because the status still can't be persisted.
   
   I think we probably need a http endpoint that can set the bookie to 
forceReadOnly mode or cancel the forceReadOnly mode, so that we can choose to 
ignore any further transition operation. In order to do that, we need to 
persist this forceReadOnly option on disk, so that when the bookie get 
restarted, it should start with --readonly option.
   
   
 

This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

BP-11: New BookKeeper Website

[Sijie Guo]

Hi all,

Luc Perkins is helping on a new bookkeeper documentation site for BP-11.
Although it is still WIP, we'd like to first share the beta version with
the community to get some feedbacks.

The new website: https://lucperkins.github.io/bookkeeper/
The pull request (still WIP) : https://github.com/apache/bookkeeper/pull/293

- Sijie

[GitHub] sijie commented on issue #294: BP-11: New BooKeeper Website

[git]

sijie commented on issue #294: BP-11: New BooKeeper Website
URL: https://github.com/apache/bookkeeper/issues/294#issuecomment-317837421
 
 
   ^ @lucperkins 
 

This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] sijie opened a new issue #294: BP-11: New BooKeeper Website

[git]

sijie opened a new issue #294: BP-11: New BooKeeper Website
URL: https://github.com/apache/bookkeeper/issues/294
 
 
   **FEATURE REQUEST**
   
   1. Please describe the feature you are requesting.
   
   - new bookkeeper website/documentation (move the build from Apache CMS to 
Jekyll)
   
   This is the master ticket for 
https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=71012301
   
   2. Indicate the importance of this issue to you (blocker, must-have, 
should-have, nice-to-have). Are you currently using any workarounds to address 
this issue?
   
   *nice-to-have*
   
   3. Provide any additional detail on your proposed use case for this feature.
   
   N/A
 

This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] sijie commented on issue #197: BOOKKEEPER-974: Add an official bookkeeper docker image

[git]

sijie commented on issue #197: BOOKKEEPER-974: Add an official bookkeeper 
docker image
URL: https://github.com/apache/bookkeeper/pull/197#issuecomment-317831799
 
 
   @caiok 
   
   Let me try to summarize here. There are two approaches: 
   
   1. the approach suggested in this pull request. a summary for this approach.
   
   - use the docker hub autobuilds.
   - download released package in docker build
   - maintain different docker files for different versions
   
   pro: you can use docker hub autobuilds the images
   cons: 
   - you have to manage the docker hub account in order to build the images, 
because you have to create the builds in docker hub side. in apache, we might 
not have access to apache docker account.
   
   2. the approach other people suggested in this pull request.
   
   - use the apache/travis ci build, build the image along as part of the CI
   - maintain only one copy of docker files for each release/branches.
   - push the ci built image to apache docker hub account (e.g. docker push)
   
   pro: the image is built as part of CI and aligned with apache release. and 
we can simply use an API token from apache docker account to push built image 
and tag them.
   cons: managing versions might be not as easy as the first approach, although 
I am not very sure.
   
   
   Approach 2) is aligned with apache release and it is easy to configure if we 
want to push to apache docker account. 1) will be a bit tricky, not sure how we 
can create an autobuild in apache docker account.
   
   Hope this summary is clear for the people involved in this pull request. 
   
 

This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] lucperkins opened a new pull request #293: Beta version of the new website (work in progress)

[git]

lucperkins opened a new pull request #293: Beta version of the new website 
(work in progress)
URL: https://github.com/apache/bookkeeper/pull/293
 
 
   
 

This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

Re: EnsemblePlacementPolicy Public API breaking changes

[Enrico Olivelli]

Il mar 25 lug 2017, 19:08 Sijie Guo  ha scritto:

> On Tue, Jul 25, 2017 at 9:24 AM, Enrico Olivelli 
> wrote:
>
> > Il mar 25 lug 2017, 18:12 Sijie Guo  ha scritto:
> >
> > > Let me try to summarize the comments here (also as a reference for
> other
> > > people). This basically is comprised of two questions:
> > >
> > > 1) how do we handle the 'binary backward compatibility' for drop-in
> > upgrade
> > > in applications using only one bk version?
> > >
> > >  ideally if we change any method signatures in public API, it is a
> > breaking
> > > change. we should do this kind of change in a major version release.
> > >
> > > 4.5 is sort of a major version release because it attempts to merge
> > changes
> > > accumulated for years from multiple branches. Ideally we should bump
> the
> > > release version to 5.0. but since it is already close to the release
> > date,
> > > I will prefer stick the version number to 4.5 but mark it as a major
> > > release includes public interface changes (e.g. netty 4 upgrade,
> bytebuf
> > > introduced, ensemble placement policy interface changed).
> > >
> > > method overloading should be able keep such binary backward
> compatibility
> > > in general.
> > >
> > > 2) how do we handle the 'binary backward compatibility' for drop-in
> > upgrade
> > > in applications using multiple bk versions?
> > >
> > > This is tricky because it is really out of the scope of a single
> project.
> > > It is typically a problem of JVM loading jars. Shading is more a
> solution
> > > for 2).
> > >
> > >
> > > If your problem is 1), method overloading should be able to take care
> of
> > > it. You mentioned you tried method overloading, do you mind pointing me
> > > your code change.
> > > If your problem is 2), I do not see an easy way to address it even with
> > > your proposal.
> > >
> > >
> > > Other comments inline:
> > >
> > >
> > > On Tue, Jul 25, 2017 at 3:43 AM, Enrico Olivelli 
> > > wrote:
> > >
> > > > 2017-07-24 22:54 GMT+02:00 Sijie Guo :
> > > >
> > > > > On Mon, Jul 24, 2017 at 1:29 PM, Enrico Olivelli <
> > eolive...@gmail.com>
> > > > > wrote:
> > > > >
> > > > > > Il lun 24 lug 2017, 19:39 Sijie Guo  ha
> > scritto:
> > > > > >
> > > > > > > On Mon, Jul 24, 2017 at 5:22 AM, Enrico Olivelli <
> > > > eolive...@gmail.com>
> > > > > > > wrote:
> > > > > > >
> > > > > > > > Hi,
> > > > > > > > we are going to release 4.5 soon, in this release we changed
> > > > > > > > EnsemblePlacementPolicy interface.
> > > > > > > >
> > > > > > > > It you are using a "custom" EnsemblePlacementPolicy in 4.4
> and
> > > you
> > > > > just
> > > > > > > > switch the bookeeeper-server JAR on the classpath your
> > > application
> > > > > > won't
> > > > > > > > run.
> > > > > > > >
> > > > > > > > I already got that problem and I needed to implement some
> > > "support
> > > > > 4.5"
> > > > > > > > option in my projects which essentially tells "do not use a
> > > custom
> > > > > > > policy",
> > > > > > > > this is very bad because there is no way to apply the custom
> > > rules
> > > > > > > required
> > > > > > > > by the project.
> > > > > > > >
> > > > > > >
> > > > > > > Do you mean the new methods introduced in placement policy? Or
> > > > methods
> > > > > > that
> > > > > > > whose signatures are changed?
> > > > > > >
> > > > > >
> > > > > > The new signatures
> > > > > >
> > > > > > >
> > > > > > > I believe the new methods 'reorderSequence' is disabled by
> > default
> > > > > unless
> > > > > > > you enable it explicitly.
> > > > > > >
> > > > > > > For methods that whose signatures were changed, we can add the
> > old
> > > > > > methods
> > > > > > > back and create overrides to keep the binary backward
> compatible.
> > > > > > >
> > > > > >
> > > > > > I don't know if this cam work. I have already tried. I will
> double
> > > > check.
> > > > > > Anyway it will be a bit messy
> > > > > >
> > > > > > >
> > > > > > > However I am not sure if this works because the placement
> policy
> > is
> > > > > > > instantiated and loaded by reflection. Typically when you
> > upgrade a
> > > > > > > version, you have to compile it first, no?
> > > > > > >
> > > > > >
> > > > > > I have several libraries which use bk and they are built on 4.4
> and
> > > > they
> > > > > > are working together in the same classpath.
> > > > > > For instance now I am going to change one of them in order to
> > > leverage
> > > > a
> > > > > > new 4.5  feature, like readUnconfirmedEntries just as an example,
> > so
> > > I
> > > > > need
> > > > > > to switch to 4.5 on the classpath but the other projects won't
> run.
> > > > > >
> > > > >
> > > > > Are you talking about mixing 4.4 and 4.5 together?
> > > > >
> > > >
> > > > Yes, as for most widely used libraries (like ZooKeeper) it is common
> to
> > > put
> > > > a "new version" or the library on a application compiled for a
> previous
> > 

Re: EnsemblePlacementPolicy Public API breaking changes

[Sijie Guo]

On Tue, Jul 25, 2017 at 9:24 AM, Enrico Olivelli 
wrote:

> Il mar 25 lug 2017, 18:12 Sijie Guo  ha scritto:
>
> > Let me try to summarize the comments here (also as a reference for other
> > people). This basically is comprised of two questions:
> >
> > 1) how do we handle the 'binary backward compatibility' for drop-in
> upgrade
> > in applications using only one bk version?
> >
> >  ideally if we change any method signatures in public API, it is a
> breaking
> > change. we should do this kind of change in a major version release.
> >
> > 4.5 is sort of a major version release because it attempts to merge
> changes
> > accumulated for years from multiple branches. Ideally we should bump the
> > release version to 5.0. but since it is already close to the release
> date,
> > I will prefer stick the version number to 4.5 but mark it as a major
> > release includes public interface changes (e.g. netty 4 upgrade, bytebuf
> > introduced, ensemble placement policy interface changed).
> >
> > method overloading should be able keep such binary backward compatibility
> > in general.
> >
> > 2) how do we handle the 'binary backward compatibility' for drop-in
> upgrade
> > in applications using multiple bk versions?
> >
> > This is tricky because it is really out of the scope of a single project.
> > It is typically a problem of JVM loading jars. Shading is more a solution
> > for 2).
> >
> >
> > If your problem is 1), method overloading should be able to take care of
> > it. You mentioned you tried method overloading, do you mind pointing me
> > your code change.
> > If your problem is 2), I do not see an easy way to address it even with
> > your proposal.
> >
> >
> > Other comments inline:
> >
> >
> > On Tue, Jul 25, 2017 at 3:43 AM, Enrico Olivelli 
> > wrote:
> >
> > > 2017-07-24 22:54 GMT+02:00 Sijie Guo :
> > >
> > > > On Mon, Jul 24, 2017 at 1:29 PM, Enrico Olivelli <
> eolive...@gmail.com>
> > > > wrote:
> > > >
> > > > > Il lun 24 lug 2017, 19:39 Sijie Guo  ha
> scritto:
> > > > >
> > > > > > On Mon, Jul 24, 2017 at 5:22 AM, Enrico Olivelli <
> > > eolive...@gmail.com>
> > > > > > wrote:
> > > > > >
> > > > > > > Hi,
> > > > > > > we are going to release 4.5 soon, in this release we changed
> > > > > > > EnsemblePlacementPolicy interface.
> > > > > > >
> > > > > > > It you are using a "custom" EnsemblePlacementPolicy in 4.4 and
> > you
> > > > just
> > > > > > > switch the bookeeeper-server JAR on the classpath your
> > application
> > > > > won't
> > > > > > > run.
> > > > > > >
> > > > > > > I already got that problem and I needed to implement some
> > "support
> > > > 4.5"
> > > > > > > option in my projects which essentially tells "do not use a
> > custom
> > > > > > policy",
> > > > > > > this is very bad because there is no way to apply the custom
> > rules
> > > > > > required
> > > > > > > by the project.
> > > > > > >
> > > > > >
> > > > > > Do you mean the new methods introduced in placement policy? Or
> > > methods
> > > > > that
> > > > > > whose signatures are changed?
> > > > > >
> > > > >
> > > > > The new signatures
> > > > >
> > > > > >
> > > > > > I believe the new methods 'reorderSequence' is disabled by
> default
> > > > unless
> > > > > > you enable it explicitly.
> > > > > >
> > > > > > For methods that whose signatures were changed, we can add the
> old
> > > > > methods
> > > > > > back and create overrides to keep the binary backward compatible.
> > > > > >
> > > > >
> > > > > I don't know if this cam work. I have already tried. I will double
> > > check.
> > > > > Anyway it will be a bit messy
> > > > >
> > > > > >
> > > > > > However I am not sure if this works because the placement policy
> is
> > > > > > instantiated and loaded by reflection. Typically when you
> upgrade a
> > > > > > version, you have to compile it first, no?
> > > > > >
> > > > >
> > > > > I have several libraries which use bk and they are built on 4.4 and
> > > they
> > > > > are working together in the same classpath.
> > > > > For instance now I am going to change one of them in order to
> > leverage
> > > a
> > > > > new 4.5  feature, like readUnconfirmedEntries just as an example,
> so
> > I
> > > > need
> > > > > to switch to 4.5 on the classpath but the other projects won't run.
> > > > >
> > > >
> > > > Are you talking about mixing 4.4 and 4.5 together?
> > > >
> > >
> > > Yes, as for most widely used libraries (like ZooKeeper) it is common to
> > put
> > > a "new version" or the library on a application compiled for a previous
> > > version.
> > > I see that sometimes it is not possible, and for this reason we use
> > 'major
> > > versions' vs 'minor versions'.
> > > When possible I think it is best not to introduce such breaking
> changes.
> > >
> >
> > I agree with you. That's the purpose of 'major version' and 'minor
> > version'. However,
> > 4.5 is a special case which it was an attempt to 

[GitHub] yzang commented on a change in pull request #278: BOOKKEEPER-1100: Add module for Bookkeeper Http Endpoint

[git]

yzang commented on a change in pull request #278: BOOKKEEPER-1100: Add module 
for Bookkeeper Http Endpoint
URL: https://github.com/apache/bookkeeper/pull/278#discussion_r129364263
 
 

 ##
 File path: 
bookkeeper-http/vertx-http-server/src/main/java/org/apache/bookkeeper/http/vertx/package-info.java
 ##
 @@ -0,0 +1,20 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with this
+ * work for additional information regarding copyright ownership. The ASF
+ * licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations 
under
+ * the License.
+ */
+/**
+ * @TODO: Write JavaDoc comment {@link 
https://github.com/apache/bookkepeer/issues/247}
 
 Review comment:
   Sure, I'll add some javadoc
 

This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] yzang commented on a change in pull request #278: BOOKKEEPER-1100: Add module for Bookkeeper Http Endpoint

[git]

yzang commented on a change in pull request #278: BOOKKEEPER-1100: Add module 
for Bookkeeper Http Endpoint
URL: https://github.com/apache/bookkeeper/pull/278#discussion_r129363913
 
 

 ##
 File path: bookkeeper-server/pom.xml
 ##
 @@ -217,11 +217,21 @@
   ${netty.version}
 
 
-org.apache.hadoop
-hadoop-minikdc
-2.7.3
-test
-  
+  org.apache.hadoop
+  hadoop-minikdc
+  2.7.3
+  test
+
+
+  org.json
 
 Review comment:
   sure, I'll try jsonb or jackson, thanks.
 

This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] yzang commented on a change in pull request #278: BOOKKEEPER-1100: Add module for Bookkeeper Http Endpoint

[git]

yzang commented on a change in pull request #278: BOOKKEEPER-1100: Add module 
for Bookkeeper Http Endpoint
URL: https://github.com/apache/bookkeeper/pull/278#discussion_r129363712
 
 

 ##
 File path: bookkeeper-server/pom.xml
 ##
 @@ -217,11 +217,21 @@
   ${netty.version}
 
 
-org.apache.hadoop
-hadoop-minikdc
-2.7.3
-test
-  
+  org.apache.hadoop
+  hadoop-minikdc
+  2.7.3
+  test
+
+
+  org.json
+  json
+  20160810
+
+
+  org.apache.bookkeeper.http
+  http-server
+  4.5.0-SNAPSHOT
 
 Review comment:
   Ah, good catch, I'll fix it.
 

This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] caiok commented on issue #197: BOOKKEEPER-974: Add an official bookkeeper docker image

[git]

caiok commented on issue #197: BOOKKEEPER-974: Add an official bookkeeper 
docker image
URL: https://github.com/apache/bookkeeper/pull/197#issuecomment-317797541
 
 
   @sijie Thanks for the reply. We don't push images on dockerhub, dockerhub 
checkout our dockerfiles and builds them. Check [this 
page](https://docs.docker.com/docker-hub/builds/#understand-the-build-process) 
in order to have an idea of the process.
   Current dockerfile will be useless for master version until the package will 
be released somewhere (but we can create an onbuild dockerfile that checkout 
and build the code in the container).
   Anyway, I'm fine to let version x.y.z of docker build to live on branch 
x.y.z, if this is the prevalent preference (there are some inconveniences that 
I explained to Jia, though) 
 

This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

Re: [DISCUSS] Slack Channel for BookKeeper

[Henry Saputra]

Is Apache BookKeeper has its own Slack team? If it has then we could just
add a channel for DistLog.

- Henry

On Mon, Jul 24, 2017 at 11:42 PM, Enrico Olivelli 
wrote:

> Awesome +1
>
> Il mar 25 lug 2017, 05:39 Dustin Castor 
> ha
> scritto:
>
> > Agreed! Or an IRC.
> >
> > On Monday, July 24, 2017, 6:50:22 PM PDT, Jia Zhai 
> > wrote:
> >
> >  It is great to have a slack channel. It make things more effective and
> > smooth.
> >
> > On Tue, Jul 25, 2017 at 8:11 AM, Sijie Guo  wrote:
> >
> > > Hi all,
> > >
> > > What do you guys all think about having a dedicated slack channel for
> > > informal discussion for the community? There are a handful of Apache
> > > projects are doing that already, there are also ways to have a bot that
> > > sends daily digest of the conversation to the mailing lists (to keep
> the
> > > records in ASF infrastructure).
> > >
> > > As the followup steps for merging DL into BookKeeper, we are
> transferring
> > > the DL slack channel to BookKeeper PMC. We can just make it a BK slack
> > > channel, and have different channels under it for different
> discussions.
> > >
> > > Thoughts?
> > >
> > > - Sijie
> > >
>
> --
>
>
> -- Enrico Olivelli
>

Re: EnsemblePlacementPolicy Public API breaking changes

[Enrico Olivelli]

Il mar 25 lug 2017, 18:12 Sijie Guo  ha scritto:

> Let me try to summarize the comments here (also as a reference for other
> people). This basically is comprised of two questions:
>
> 1) how do we handle the 'binary backward compatibility' for drop-in upgrade
> in applications using only one bk version?
>
>  ideally if we change any method signatures in public API, it is a breaking
> change. we should do this kind of change in a major version release.
>
> 4.5 is sort of a major version release because it attempts to merge changes
> accumulated for years from multiple branches. Ideally we should bump the
> release version to 5.0. but since it is already close to the release date,
> I will prefer stick the version number to 4.5 but mark it as a major
> release includes public interface changes (e.g. netty 4 upgrade, bytebuf
> introduced, ensemble placement policy interface changed).
>
> method overloading should be able keep such binary backward compatibility
> in general.
>
> 2) how do we handle the 'binary backward compatibility' for drop-in upgrade
> in applications using multiple bk versions?
>
> This is tricky because it is really out of the scope of a single project.
> It is typically a problem of JVM loading jars. Shading is more a solution
> for 2).
>
>
> If your problem is 1), method overloading should be able to take care of
> it. You mentioned you tried method overloading, do you mind pointing me
> your code change.
> If your problem is 2), I do not see an easy way to address it even with
> your proposal.
>
>
> Other comments inline:
>
>
> On Tue, Jul 25, 2017 at 3:43 AM, Enrico Olivelli 
> wrote:
>
> > 2017-07-24 22:54 GMT+02:00 Sijie Guo :
> >
> > > On Mon, Jul 24, 2017 at 1:29 PM, Enrico Olivelli 
> > > wrote:
> > >
> > > > Il lun 24 lug 2017, 19:39 Sijie Guo  ha scritto:
> > > >
> > > > > On Mon, Jul 24, 2017 at 5:22 AM, Enrico Olivelli <
> > eolive...@gmail.com>
> > > > > wrote:
> > > > >
> > > > > > Hi,
> > > > > > we are going to release 4.5 soon, in this release we changed
> > > > > > EnsemblePlacementPolicy interface.
> > > > > >
> > > > > > It you are using a "custom" EnsemblePlacementPolicy in 4.4 and
> you
> > > just
> > > > > > switch the bookeeeper-server JAR on the classpath your
> application
> > > > won't
> > > > > > run.
> > > > > >
> > > > > > I already got that problem and I needed to implement some
> "support
> > > 4.5"
> > > > > > option in my projects which essentially tells "do not use a
> custom
> > > > > policy",
> > > > > > this is very bad because there is no way to apply the custom
> rules
> > > > > required
> > > > > > by the project.
> > > > > >
> > > > >
> > > > > Do you mean the new methods introduced in placement policy? Or
> > methods
> > > > that
> > > > > whose signatures are changed?
> > > > >
> > > >
> > > > The new signatures
> > > >
> > > > >
> > > > > I believe the new methods 'reorderSequence' is disabled by default
> > > unless
> > > > > you enable it explicitly.
> > > > >
> > > > > For methods that whose signatures were changed, we can add the old
> > > > methods
> > > > > back and create overrides to keep the binary backward compatible.
> > > > >
> > > >
> > > > I don't know if this cam work. I have already tried. I will double
> > check.
> > > > Anyway it will be a bit messy
> > > >
> > > > >
> > > > > However I am not sure if this works because the placement policy is
> > > > > instantiated and loaded by reflection. Typically when you upgrade a
> > > > > version, you have to compile it first, no?
> > > > >
> > > >
> > > > I have several libraries which use bk and they are built on 4.4 and
> > they
> > > > are working together in the same classpath.
> > > > For instance now I am going to change one of them in order to
> leverage
> > a
> > > > new 4.5  feature, like readUnconfirmedEntries just as an example, so
> I
> > > need
> > > > to switch to 4.5 on the classpath but the other projects won't run.
> > > >
> > >
> > > Are you talking about mixing 4.4 and 4.5 together?
> > >
> >
> > Yes, as for most widely used libraries (like ZooKeeper) it is common to
> put
> > a "new version" or the library on a application compiled for a previous
> > version.
> > I see that sometimes it is not possible, and for this reason we use
> 'major
> > versions' vs 'minor versions'.
> > When possible I think it is best not to introduce such breaking changes.
> >
>
> I agree with you. That's the purpose of 'major version' and 'minor
> version'. However,
> 4.5 is a special case which it was an attempt to merge multiple branches
> together.
> Although it doesn't break the protocol backward compatibility and code
> compatibility
> in public client API, it does change a bit on some interfaces (for example
> the placement policy interface).
>

Yes, we did it for the AuthPlugin API too.


>
> >
> >
> >
> >
> >
> > >
> > >
> > > > For the server side this is not a problem but on 

[GitHub] sijie commented on a change in pull request #278: BOOKKEEPER-1100: Add module for Bookkeeper Http Endpoint

[git]

sijie commented on a change in pull request #278: BOOKKEEPER-1100: Add module 
for Bookkeeper Http Endpoint
URL: https://github.com/apache/bookkeeper/pull/278#discussion_r129353429
 
 

 ##
 File path: 
bookkeeper-http/http-server/src/main/java/org/apache/bookkeeper/http/HttpServerLoader.java
 ##
 @@ -0,0 +1,67 @@
+/**
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+package org.apache.bookkeeper.http;
+
+import java.lang.reflect.Constructor;
+import java.lang.reflect.InvocationTargetException;
+
+import org.apache.commons.configuration.Configuration;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Class to load and instantiate http server from config.
+ */
+public class HttpServerLoader {
+
+static final Logger LOG = LoggerFactory.getLogger(HttpServerLoader.class);
+
+public static final String HTTP_SERVER_CLASS = "httpServerClass";
+static HttpServer server = null;
+
+public static void loadHttpServer(Configuration conf) {
+String className = conf.getString(HTTP_SERVER_CLASS);
+if (className != null) {
+try {
+Class cls = Class.forName(className);
 
 Review comment:
   @eolivelli we should refactor the module layout, but I would defer that to 
next release.
 

This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] sijie commented on issue #278: BOOKKEEPER-1100: Add module for Bookkeeper Http Endpoint

[git]

sijie commented on issue #278: BOOKKEEPER-1100: Add module for Bookkeeper Http 
Endpoint
URL: https://github.com/apache/bookkeeper/pull/278#issuecomment-317789926
 
 
   >> "I wonder if we really need to implement so many variants of the http 
server or simply implement one"
   
   different organizations might have different preferences, just like stats 
providers. so I do not see a problem for that. a simple implementation can be 
the default one.
 

This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

Re: EnsemblePlacementPolicy Public API breaking changes

[Sijie Guo]

Let me try to summarize the comments here (also as a reference for other
people). This basically is comprised of two questions:

1) how do we handle the 'binary backward compatibility' for drop-in upgrade
in applications using only one bk version?

 ideally if we change any method signatures in public API, it is a breaking
change. we should do this kind of change in a major version release.

4.5 is sort of a major version release because it attempts to merge changes
accumulated for years from multiple branches. Ideally we should bump the
release version to 5.0. but since it is already close to the release date,
I will prefer stick the version number to 4.5 but mark it as a major
release includes public interface changes (e.g. netty 4 upgrade, bytebuf
introduced, ensemble placement policy interface changed).

method overloading should be able keep such binary backward compatibility
in general.

2) how do we handle the 'binary backward compatibility' for drop-in upgrade
in applications using multiple bk versions?

This is tricky because it is really out of the scope of a single project.
It is typically a problem of JVM loading jars. Shading is more a solution
for 2).


If your problem is 1), method overloading should be able to take care of
it. You mentioned you tried method overloading, do you mind pointing me
your code change.
If your problem is 2), I do not see an easy way to address it even with
your proposal.


Other comments inline:


On Tue, Jul 25, 2017 at 3:43 AM, Enrico Olivelli 
wrote:

> 2017-07-24 22:54 GMT+02:00 Sijie Guo :
>
> > On Mon, Jul 24, 2017 at 1:29 PM, Enrico Olivelli 
> > wrote:
> >
> > > Il lun 24 lug 2017, 19:39 Sijie Guo  ha scritto:
> > >
> > > > On Mon, Jul 24, 2017 at 5:22 AM, Enrico Olivelli <
> eolive...@gmail.com>
> > > > wrote:
> > > >
> > > > > Hi,
> > > > > we are going to release 4.5 soon, in this release we changed
> > > > > EnsemblePlacementPolicy interface.
> > > > >
> > > > > It you are using a "custom" EnsemblePlacementPolicy in 4.4 and you
> > just
> > > > > switch the bookeeeper-server JAR on the classpath your application
> > > won't
> > > > > run.
> > > > >
> > > > > I already got that problem and I needed to implement some "support
> > 4.5"
> > > > > option in my projects which essentially tells "do not use a custom
> > > > policy",
> > > > > this is very bad because there is no way to apply the custom rules
> > > > required
> > > > > by the project.
> > > > >
> > > >
> > > > Do you mean the new methods introduced in placement policy? Or
> methods
> > > that
> > > > whose signatures are changed?
> > > >
> > >
> > > The new signatures
> > >
> > > >
> > > > I believe the new methods 'reorderSequence' is disabled by default
> > unless
> > > > you enable it explicitly.
> > > >
> > > > For methods that whose signatures were changed, we can add the old
> > > methods
> > > > back and create overrides to keep the binary backward compatible.
> > > >
> > >
> > > I don't know if this cam work. I have already tried. I will double
> check.
> > > Anyway it will be a bit messy
> > >
> > > >
> > > > However I am not sure if this works because the placement policy is
> > > > instantiated and loaded by reflection. Typically when you upgrade a
> > > > version, you have to compile it first, no?
> > > >
> > >
> > > I have several libraries which use bk and they are built on 4.4 and
> they
> > > are working together in the same classpath.
> > > For instance now I am going to change one of them in order to leverage
> a
> > > new 4.5  feature, like readUnconfirmedEntries just as an example, so I
> > need
> > > to switch to 4.5 on the classpath but the other projects won't run.
> > >
> >
> > Are you talking about mixing 4.4 and 4.5 together?
> >
>
> Yes, as for most widely used libraries (like ZooKeeper) it is common to put
> a "new version" or the library on a application compiled for a previous
> version.
> I see that sometimes it is not possible, and for this reason we use 'major
> versions' vs 'minor versions'.
> When possible I think it is best not to introduce such breaking changes.
>

I agree with you. That's the purpose of 'major version' and 'minor
version'. However,
4.5 is a special case which it was an attempt to merge multiple branches
together.
Although it doesn't break the protocol backward compatibility and code
compatibility
in public client API, it does change a bit on some interfaces (for example
the placement policy interface).


>
>
>
>
>
> >
> >
> > > For the server side this is not a problem but on the client it is a
> real
> > > production problem.
> > >
> >
> > Can you clarify more specifically on this? Not very sure I understand
> your
> > problem and what are you going to achieve.
> >
> >
> Usually you have full control on bookies, but it is not always possible to
> have full control of applications assembler from different components.
> Example:
> - I have Lib 1 v1 which is 

[GitHub] eolivelli commented on a change in pull request #276: Issue-271 LedgerHandle#readEntries leaks ByteBufs

[git]

eolivelli commented on a change in pull request #276: Issue-271 
LedgerHandle#readEntries leaks ByteBufs
URL: https://github.com/apache/bookkeeper/pull/276#discussion_r129337447
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerEntry.java
 ##
 @@ -53,22 +54,49 @@ public long getLength() {
 return length;
 }
 
+/**
+ * Returns the content of the entry.
+ * This method can be called only once.
+ * While using v2 wire protocol this method will automatically release the 
internal ByteBuf
+ * @return
+ */
 public byte[] getEntry() {
+if (data == null) {
 
 Review comment:
   @sijie I will update the patch as soon as possibile.
   Thank you
 

This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] sijie commented on a change in pull request #276: Issue-271 LedgerHandle#readEntries leaks ByteBufs

[git]

sijie commented on a change in pull request #276: Issue-271 
LedgerHandle#readEntries leaks ByteBufs
URL: https://github.com/apache/bookkeeper/pull/276#discussion_r129336829
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerEntry.java
 ##
 @@ -53,22 +54,49 @@ public long getLength() {
 return length;
 }
 
+/**
+ * Returns the content of the entry.
+ * This method can be called only once.
+ * While using v2 wire protocol this method will automatically release the 
internal ByteBuf
+ * @return
+ */
 public byte[] getEntry() {
+if (data == null) {
 
 Review comment:
   @eolivelli you don't have to revert it. it is good to make it clear. I just 
want to clarify to make things clear at this pull request. for assertion, I 
would prefer using guava Preconditions.checkState(...).
 

This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] sijie commented on issue #197: BOOKKEEPER-974: Add an official bookkeeper docker image

[git]

sijie commented on issue #197: BOOKKEEPER-974: Add an official bookkeeper 
docker image
URL: https://github.com/apache/bookkeeper/pull/197#issuecomment-317770478
 
 
   @caiok : regarding the versioning here, I am not sure whether we need to 
keep the version here. Ideally the docker image should be built along with an 
apache release. For example when we cut an apache release 4.5.0, along with the 
release procedure, we will build the 4.5.0 image and push it to apache docker 
hub. The docker files for 4.5.0 will live in the branch 4.5.0. The docker files 
in master will evolve with the changes in next release (for example 4.6.0). 
That means the docker files for image 4.5.0 will live only in branch 4.5.0 and 
subsequent changes to 4.5.0 will happen only in branch 4.5.0 and new changes 
will live in master. How does that sound?
   
   I see a few issues created for followup actions. I am fine with merging this 
pull request and iterate after that if we can achieve basic consensus here.
   
   @fpj @merlimat : since you were reviewing this pull request, please try to 
take a look at this pull request when you have time. I'd like to move this 
forward with 4.5.0 release.
   
 

This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] eolivelli commented on a change in pull request #278: BOOKKEEPER-1100: Add module for Bookkeeper Http Endpoint

[git]

eolivelli commented on a change in pull request #278: BOOKKEEPER-1100: Add 
module for Bookkeeper Http Endpoint
URL: https://github.com/apache/bookkeeper/pull/278#discussion_r129330400
 
 

 ##
 File path: bookkeeper-server/pom.xml
 ##
 @@ -217,11 +217,21 @@
   ${netty.version}
 
 
-org.apache.hadoop
-hadoop-minikdc
-2.7.3
-test
-  
+  org.apache.hadoop
+  hadoop-minikdc
+  2.7.3
+  test
+
+
+  org.json
+  json
+  20160810
+
+
+  org.apache.bookkeeper.http
+  http-server
+  4.5.0-SNAPSHOT
 
 Review comment:
   use ${project.version}
 

This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] eolivelli commented on a change in pull request #278: BOOKKEEPER-1100: Add module for Bookkeeper Http Endpoint

[git]

eolivelli commented on a change in pull request #278: BOOKKEEPER-1100: Add 
module for Bookkeeper Http Endpoint
URL: https://github.com/apache/bookkeeper/pull/278#discussion_r129329168
 
 

 ##
 File path: 
bookkeeper-http/http-server/src/main/java/org/apache/bookkeeper/http/HttpServerLoader.java
 ##
 @@ -0,0 +1,67 @@
+/**
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+package org.apache.bookkeeper.http;
+
+import java.lang.reflect.Constructor;
+import java.lang.reflect.InvocationTargetException;
+
+import org.apache.commons.configuration.Configuration;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Class to load and instantiate http server from config.
+ */
+public class HttpServerLoader {
+
+static final Logger LOG = LoggerFactory.getLogger(HttpServerLoader.class);
+
+public static final String HTTP_SERVER_CLASS = "httpServerClass";
+static HttpServer server = null;
+
+public static void loadHttpServer(Configuration conf) {
+String className = conf.getString(HTTP_SERVER_CLASS);
+if (className != null) {
+try {
+Class cls = Class.forName(className);
 
 Review comment:
   we should use ReflectionUtils but I think it is not possible as it is in 
bookkeeper-server
   @sijie  should we create a new minor project "bookeeper-utils" ?
 

This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] eolivelli commented on a change in pull request #278: BOOKKEEPER-1100: Add module for Bookkeeper Http Endpoint

[git]

eolivelli commented on a change in pull request #278: BOOKKEEPER-1100: Add 
module for Bookkeeper Http Endpoint
URL: https://github.com/apache/bookkeeper/pull/278#discussion_r129329619
 
 

 ##
 File path: 
bookkeeper-http/http-server/src/main/java/org/apache/bookkeeper/http/service/ServiceRequest.java
 ##
 @@ -0,0 +1,71 @@
+/**
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+package org.apache.bookkeeper.http.service;
+
+import java.util.HashMap;
+import java.util.Map;
+
+import org.apache.bookkeeper.http.HttpServer;
+
+/**
+ * A wrapper class that wrap a http request into a class which
+ * can then be passed into the service.
+ */
+public class ServiceRequest {
+private String body;
+private HttpServer.Method method = HttpServer.Method.GET;
+private Map params = new HashMap();
 
 Review comment:
   it is better to declare explicit types of the param, maybe  o 

 

This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] eolivelli commented on a change in pull request #278: BOOKKEEPER-1100: Add module for Bookkeeper Http Endpoint

[git]

eolivelli commented on a change in pull request #278: BOOKKEEPER-1100: Add 
module for Bookkeeper Http Endpoint
URL: https://github.com/apache/bookkeeper/pull/278#discussion_r129330300
 
 

 ##
 File path: 
bookkeeper-http/vertx-http-server/src/main/java/org/apache/bookkeeper/http/vertx/package-info.java
 ##
 @@ -0,0 +1,20 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with this
+ * work for additional information regarding copyright ownership. The ASF
+ * licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations 
under
+ * the License.
+ */
+/**
+ * @TODO: Write JavaDoc comment {@link 
https://github.com/apache/bookkepeer/issues/247}
 
 Review comment:
   link is broken, anyway we just fixed issue 
https://github.com/apache/bookkeeper/issues/247 so please add a minimal javadoc
 

This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] eolivelli commented on a change in pull request #278: BOOKKEEPER-1100: Add module for Bookkeeper Http Endpoint

[git]

eolivelli commented on a change in pull request #278: BOOKKEEPER-1100: Add 
module for Bookkeeper Http Endpoint
URL: https://github.com/apache/bookkeeper/pull/278#discussion_r129331345
 
 

 ##
 File path: bookkeeper-server/pom.xml
 ##
 @@ -217,11 +217,21 @@
   ${netty.version}
 
 
-org.apache.hadoop
-hadoop-minikdc
-2.7.3
-test
-  
+  org.apache.hadoop
+  hadoop-minikdc
+  2.7.3
+  test
+
+
+  org.json
 
 Review comment:
   please consider using http://json-b.net/ which is the current (brand new) 
standard for Java-JSON mapping.
   org.json is really bad.
   otherwise we could  use Jackson Mapper, which is actually the best
 

This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] sijie commented on a change in pull request #276: Issue-271 LedgerHandle#readEntries leaks ByteBufs

[git]

sijie commented on a change in pull request #276: Issue-271 
LedgerHandle#readEntries leaks ByteBufs
URL: https://github.com/apache/bookkeeper/pull/276#discussion_r129331061
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerEntry.java
 ##
 @@ -53,22 +54,49 @@ public long getLength() {
 return length;
 }
 
+/**
+ * Returns the content of the entry.
+ * This method can be called only once.
+ * While using v2 wire protocol this method will automatically release the 
internal ByteBuf
+ * @return
+ */
 public byte[] getEntry() {
+if (data == null) {
 
 Review comment:
   @eolivelli 
   
   there are two separate questions here.
   
   1) are you going to change this behavior to support multiple gets?
   2) if you are not going to change this behavior, why do we need you change?
   
   The first question is more about whether to keep the behavior same as 
before. The behavior without your code change is exactly same as before. If you 
want to change this behavior, you can mark the read position, read the data and 
reset the read position. but your change doesn't seem to be this case.
   
   for 2) change, without your change, it still throws IlegalStateException 
because the bytebuf will be released twice. so I don't see a strong reason why 
do we need your change here. I am not talking about the 'if (data == null)' 
case, I am talking about the change in general. Because I do not see a strong 
reason to change this if we are not changing the behavior.
 

This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] eolivelli commented on issue #290: Add checkstyle:checkstyle to bk-merge script

[git]

eolivelli commented on issue #290: Add checkstyle:checkstyle to bk-merge script
URL: https://github.com/apache/bookkeeper/issues/290#issuecomment-317762194
 
 
   closing as won't fix. checkstyle is already bound to "validate" phase for 
every project which supports it, it is not active in bookkeeper-server
 

This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] eolivelli closed issue #290: Add checkstyle:checkstyle to bk-merge script

[git]

eolivelli closed issue #290: Add checkstyle:checkstyle to bk-merge script
URL: https://github.com/apache/bookkeeper/issues/290
 
 
   
 

This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] eolivelli commented on issue #291: Issue-290 Add checkstyle:checkstyle to bk-merge script

[git]

eolivelli commented on issue #291: Issue-290 Add checkstyle:checkstyle to 
bk-merge script
URL: https://github.com/apache/bookkeeper/pull/291#issuecomment-317761995
 
 
   @sijie sure, sorry I missed it. Usually I am working with bookkeeper-server, 
I forgot about the fact we have only in minor maven projects
 

This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] eolivelli closed pull request #291: Issue-290 Add checkstyle:checkstyle to bk-merge script

[git]

eolivelli closed pull request #291: Issue-290 Add checkstyle:checkstyle to 
bk-merge script
URL: https://github.com/apache/bookkeeper/pull/291
 
 
   
 

This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

Re: Bookie Http Endpoint

[Sijie Guo]

Dustin, Enrico,

If you guys have time, please take a look at Yiming's new pull request :
https://github.com/apache/bookkeeper/pull/278

- Sijie

On Wed, Jul 19, 2017 at 9:46 PM, Enrico Olivelli 
wrote:

> Il gio 20 lug 2017, 00:37 Sijie Guo  ha scritto:
>
> > On Wed, Jul 19, 2017 at 10:02 AM, Dustin Castor <
> > dustincas...@yahoo.com.invalid> wrote:
> >
> > > I do agree that we need a contractual way of managing the server
> > lifecycle
> > > via an interface -- identical to how we do it with stat providers.
> >
> >
> >
> >
> > > Personally I don't think we can really have contracts for each and
> every
> > > method we all want from an interface, so not so sure how far the
> > interface
> > > can go in offering uniform methods for what we all want an endpoint to
> > do.
> > > Sure, we all want to get configurations, but what if we wanted to to do
> > > something else (e.g., run-time config changes of certain parameters)
> and
> > > someone else didn't?
> > >
> >
> > I think if the methods are contributed back to the open source community,
> > we can form the superset of methods that work for most of the people.
> Does
> > this work for you?
> >
> >
> > >
> > > Regarding paths, I did like the simplicity of the Jersey
> implementation.
> > > E.g., put this right above class to denote a path:
> > > @Path("/resources")public class Rest{
> > > And this above a method:@GET
> > > @Path("/v1/configurations")
> > > @Produces(MediaType.APPLICATION_JSON)
> > > public String getConfiguration(@QueryParam("config") String config) {
> }
> > > This could be hit via something like [host]:[port]/resources/v1/
> > > configurations?config=BookiePort
> > >
> >
> > I think this can also fit in a simple server lifecycle, right?
> >
> >
> > >
> > > Agreed that ultimately so long as they work, I don't really care which
> > > library is used.
> > >
> >
> >
> > If that's the case, can we see if it can be based on Yiming's patch?
> >
> > - Keep the simple server lifecycle.
> > - Move the TwitterHttpServer and VexServer as plugins.
> >
> > Does it look like a base that can meet the minimal requirement here?
> >
>
> Yes for me
>
> Thoughts?
> >
>
> I will put all these things together in a wiki page
>
> Enrico
>
>
> > - Sijie
> >
> >
> > >
> > >
> > >
> > >
> > > On Wednesday, July 19, 2017, 2:52:38 AM PDT, Sijie Guo <
> > guosi...@gmail.com>
> > > wrote:
> > >
> > > On Wed, Jul 19, 2017 at 2:55 PM, Enrico Olivelli 
> > > wrote:
> > >
> > > > 2017-07-19 3:27 GMT+02:00 Yiming Zang :
> > > >
> > > > > Hi all,
> > > > >
> > > > > We also have a working implementation in Twitter, we use Http
> > Endpoint
> > > > > mostly for getting quorum loss information, underreplicated
> ledgers,
> > > > manage
> > > > > bookie status etc.
> > > > >
> > > > > The HTTP server in Twitter is implemented in such a way that it
> has a
> > > > > generic skeleton which can easily embed with different HTTP
> > frameworks
> > > > > (Vertx, Undertow, TwitterServer, etc), because all these frameworks
> > > have
> > > > > something in common, they all have Http Handler, we only need to
> > > > implement
> > > > > the functions for the Handler, and then bind the Handler to a
> > specific
> > > > > endpoint in the router. We intend to keep the code simple and neat,
> > > easy
> > > > to
> > > > > extend, and keep the implementation flexible. There's no limitation
> > > here.
> > > > >
> > > > > The pull request https://github.com/apache/bookkeeper/pull/210 is
> > > > > basically
> > > > > the skeleton of the HTTP server in Twitter. If this is what's
> needed
> > in
> > > > > OSS, I'm happy to keep working on the pull request and push the
> > changes
> > > > to
> > > > > master.
> > > > >
> > > >
> > > >
> > > > I think that the main point is that we need to draft a "standard"
> HTTP
> > > API
> > > > for the Bookie, then we can make several implementations of such API.
> > > > Once we have a common API it will be really easy to create tools and
> > > > integrate BK with other systems, like we are already doing with the
> > Stats
> > > > API.
> > > >
> > > > For me a great thing would be to have a ready to use HttpServlet,
> which
> > > is
> > > > the "standard" in the Java Web would and can be deployed on every
> Java
> > > Web
> > > > container.
> > > > A JAX-RS resource could be good too, but it needs more support from
> > the
> > > > container.
> > > >
> > > > In the near future we (at Diennea) wnat to start developing a global
> > > WebUI
> > > > for BookKeeper, which will show the status of the cluster, and having
> > > HTTP
> > > > endpoints in Bookie will ease this work
> > > >
> > > > Does anyone want to start a Wiki page ?
> > > >
> > >
> > > Enrico, can we hold on starting a wiki page?
> > >
> > > Currently I see three different approaches on this topic. An email
> > > discussion or a google doc might be better at this point.
> > >
> > > I think people have 

[GitHub] sijie commented on issue #291: Issue-290 Add checkstyle:checkstyle to bk-merge script

[git]

sijie commented on issue #291: Issue-290 Add checkstyle:checkstyle to bk-merge 
script
URL: https://github.com/apache/bookkeeper/pull/291#issuecomment-317760987
 
 
   Why do you need this? 'checkstyle' is in already in `validate` phase.
 

This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] sijie commented on issue #266: Issue 265: Add persistable bookie status

[git]

sijie commented on issue #266: Issue 265: Add persistable bookie status
URL: https://github.com/apache/bookkeeper/pull/266#issuecomment-317760092
 
 
   @yzang : I think the ideal override sequence of write/readonly status would 
be:
   
   1. if a bookie is started with '--readonly' option, it will be forced 
readonly (ignoring the bookie status persisted in the disks and ignoring the 
real disk usage) 
   2. if a bookie is started without '--readonly' option, it will first respect 
the status persisted in the disks (because that is the status set for operation 
purpose).
   3. if a bookie is started without '--readonly' option and there is not 
status persisted in the disks, that it will transition based on the read disk 
usage.
   
   3) is already there regarding this change, 1) is achieved by this change, 2) 
is not ideal without this approach yet, we can make the case one the http 
endpoint is merged we can enable this there.
   
   I don't think you need another status field in this file. In the 
BookieStatus, you can distinguish 2) and 3) easily, 2) is the status persisted 
while 3) is the in-memory status. the final status is if 2) is readonly, the 
final status will be readonly regarding what status is 3), if 2) is writable, 
the final status will be the status of 3). Does that sound good? @yzang 
 

This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] caiok opened a new issue #292: Docker image: setup docker automated build on Apache DockerHub account

[git]

caiok opened a new issue #292: Docker image: setup docker automated build on 
Apache DockerHub account 
URL: https://github.com/apache/bookkeeper/issues/292
 
 
   This issue is just for tracking progresses on this matter
 

This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] caiok commented on issue #288: Docker image: provide a template related way to handle different version

[git]

caiok commented on issue #288: Docker image: provide a template related way to 
handle different version
URL: https://github.com/apache/bookkeeper/issues/288#issuecomment-317750524
 
 
   Simple placeholders substitution is not enough for having a common template 
for centos and alpine. We need something that could conditionally put pieces of 
code. The simplest idea that comes to my mind is to use C preprocessor and 
instruct makefile for "compile" target platform-version dirs.
 

This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

Re: BookKeeper 4.5.0 performance regression ?

[Enrico Olivelli]

2017-07-25 15:36 GMT+02:00 Enrico Olivelli :

>
>
> 2017-07-25 13:58 GMT+02:00 Enrico Olivelli :
>
>> I noticed that the "performance" drop in my bench happens contextually to
>> the opening of several "RandomAccessFile", to .dx files.
>>
>> In my bench I continue to perform writes and after some time the overall
>> performance (latency and throughtput) "degrade"
>> while the bench is running I see that the overall number of open files
>> (with "lsof") and the number of open RandomAccessFile (using YourKit
>> profile) continue to grow.
>>
>> The mechanics in FileInfo are quite complex
>>
>
>
> I have changed setOpenFileLimit to 10 and the Bookie seems respects this
> value.
> Even if only 10 index files are kept open performances drop anyway
>


It seems that after the first checkpoint the bookie becomes "slower" but I
cannot find the reason.

Does anyone else ever noticed this fact ?
Are the 'real' bookie performances the ones recorded after the first
checkpoint?

Enrico




>
>
>
>
>
>>
>> can this be a clue on my problem ?
>>
>> This is the code of the bench
>> https://github.com/eolivelli/bookkeepers-benchs/blob/master/
>> src/test/java/BookKeeperWriteSynchClientsTest.java
>>
>> just clone from GitHub and run the test
>> on my laptop it starts with 80,0 MB/s throughput and when the "slow down"
>> occours it drops to 9 MB/s
>>
>> Any suggestion ?
>>
>> -- Enrico
>>
>>
>> 2017-07-24 22:31 GMT+02:00 Enrico Olivelli :
>>
>>>
>>>
>>> Il lun 24 lug 2017, 19:54 Venkateswara Rao Jujjuri 
>>> ha scritto:
>>>
 On Mon, Jul 24, 2017 at 3:06 AM, Enrico Olivelli 
 wrote:

 > 2017-07-21 20:37 GMT+02:00 Enrico Olivelli :
 >
 > >
 > >
 > > Il ven 21 lug 2017, 20:32 Sijie Guo  ha
 scritto:
 > >
 > >> As the discussion in a separate thread, it might be making sense to
 > check
 > >> what is the difference between using pooled allocator and unpooled
 > >> allocator using v3 protocol. Also considering comparing using heap
 > buffer
 > >> and direct buffer as well.
 > >>
 > >> I am suspecting this might contribute latency.
 > >>
 > >
 > > Yep, I am looking in this direction too.
 > > I see that many frequent writes lead to an huge use of non heap
 memory,
 > > even bounding the JVM with MaxDirectMemory with max 1GB all, the
 12GB of
 > my
 > > laptop blow away during the run of my benchmark.
 > > I suspect it is something in direct memory or something in SO
 caches.
 > > I am not very skilled in SO linux memory diagnostics
 > >
 >
 >
 > I wrote a new "write intensive" benchmark, and the only thing I have
 > noticed is that Linux is using as much RAM as possible for disk
 caches,
 > this is the expected behavior on Linux.
 >

 Yes, and this is good behavior. Why keep something unused?

>>>
>>> Yes this is why linux is better then other OSs, like Windows.
>>>


 > This is not memory allocated to the process itself.
 > There is no difference from 4.4 and 4.5 from this aspect.
 >
 > I have tried the journalRemoveFromPageCachebut it brings no
 improvement.
 >
 > I did some tests as suggested by Flavio, separating the client and the
 > bookie (even on different machines). I can say there is no "leak" nor
 on
 > client side neither on bookie side.
 >
 >
 > finally during my benchmarks I noticed that the real "performance
 drop"
 > happens after this lines of log
 >
 > lug 24, 2017 12:00:56 PM org.apache.bookkeeper.bookie.EntryLogger
 > flushRotatedLogs
 > INFO: Synced entry logger 0 to disk
 >
 > I am now investigating why after the first "flushRotatedLogs" bookie
 is
 > slowing down
 >
 >
 Thanks for the update. Eager to learn what is the culprit.

 JV


 >
 > Enrico
 >
 >
 >
 >
 > >
 > > Enrico
 > >
 > >
 > >
 > >>
 > >>
 > >>
 > >> - Sijie
 > >>
 > >> On Thu, Jul 20, 2017 at 4:49 AM, Enrico Olivelli <
 eolive...@gmail.com>
 > >> wrote:
 > >>
 > >> > Kishore, do you have news?
 > >> >
 > >> > Il ven 14 lug 2017, 09:05 Enrico Olivelli 
 ha
 > >> > scritto:
 > >> >
 > >> > > At the meeting we told the Kishore will perform some
 benchmarks on
 > his
 > >> > > side.
 > >> > > He will take a look at my code, and we are going to share the
 > results.
 > >> > > Maybe it will be possible to share the results of benchmarks
 done
 > from
 > >> > > Kishore at Salesforce too.
 > >> > >
 > >> > > The primary goal is to understand the differences between 4.4
 and
 > 4.5,
 > >> > for
 > >> > > instance if we there is a need to 

[GitHub] caiok commented on issue #197: BOOKKEEPER-974: Add an official bookkeeper docker image

[git]

caiok commented on issue #197: BOOKKEEPER-974: Add an official bookkeeper 
docker image
URL: https://github.com/apache/bookkeeper/pull/197#issuecomment-317744941
 
 
   @sijie @fpj @merlimat Any other doubts or requests for this PR? We need it 
merged before proceeding with the related issues (and any thoughts on these 
will be much appreciated too)
 

This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

Re: BookKeeper 4.5.0 performance regression ?

[Enrico Olivelli]

2017-07-25 13:58 GMT+02:00 Enrico Olivelli :

> I noticed that the "performance" drop in my bench happens contextually to
> the opening of several "RandomAccessFile", to .dx files.
>
> In my bench I continue to perform writes and after some time the overall
> performance (latency and throughtput) "degrade"
> while the bench is running I see that the overall number of open files
> (with "lsof") and the number of open RandomAccessFile (using YourKit
> profile) continue to grow.
>
> The mechanics in FileInfo are quite complex
>


I have changed setOpenFileLimit to 10 and the Bookie seems respects this
value.
Even if only 10 index files are kept open performances drop anyway





>
> can this be a clue on my problem ?
>
> This is the code of the bench
> https://github.com/eolivelli/bookkeepers-benchs/blob/master/src/test/java/
> BookKeeperWriteSynchClientsTest.java
>
> just clone from GitHub and run the test
> on my laptop it starts with 80,0 MB/s throughput and when the "slow down"
> occours it drops to 9 MB/s
>
> Any suggestion ?
>
> -- Enrico
>
>
> 2017-07-24 22:31 GMT+02:00 Enrico Olivelli :
>
>>
>>
>> Il lun 24 lug 2017, 19:54 Venkateswara Rao Jujjuri 
>> ha scritto:
>>
>>> On Mon, Jul 24, 2017 at 3:06 AM, Enrico Olivelli 
>>> wrote:
>>>
>>> > 2017-07-21 20:37 GMT+02:00 Enrico Olivelli :
>>> >
>>> > >
>>> > >
>>> > > Il ven 21 lug 2017, 20:32 Sijie Guo  ha scritto:
>>> > >
>>> > >> As the discussion in a separate thread, it might be making sense to
>>> > check
>>> > >> what is the difference between using pooled allocator and unpooled
>>> > >> allocator using v3 protocol. Also considering comparing using heap
>>> > buffer
>>> > >> and direct buffer as well.
>>> > >>
>>> > >> I am suspecting this might contribute latency.
>>> > >>
>>> > >
>>> > > Yep, I am looking in this direction too.
>>> > > I see that many frequent writes lead to an huge use of non heap
>>> memory,
>>> > > even bounding the JVM with MaxDirectMemory with max 1GB all, the
>>> 12GB of
>>> > my
>>> > > laptop blow away during the run of my benchmark.
>>> > > I suspect it is something in direct memory or something in SO caches.
>>> > > I am not very skilled in SO linux memory diagnostics
>>> > >
>>> >
>>> >
>>> > I wrote a new "write intensive" benchmark, and the only thing I have
>>> > noticed is that Linux is using as much RAM as possible for disk caches,
>>> > this is the expected behavior on Linux.
>>> >
>>>
>>> Yes, and this is good behavior. Why keep something unused?
>>>
>>
>> Yes this is why linux is better then other OSs, like Windows.
>>
>>>
>>>
>>> > This is not memory allocated to the process itself.
>>> > There is no difference from 4.4 and 4.5 from this aspect.
>>> >
>>> > I have tried the journalRemoveFromPageCachebut it brings no
>>> improvement.
>>> >
>>> > I did some tests as suggested by Flavio, separating the client and the
>>> > bookie (even on different machines). I can say there is no "leak" nor
>>> on
>>> > client side neither on bookie side.
>>> >
>>> >
>>> > finally during my benchmarks I noticed that the real "performance drop"
>>> > happens after this lines of log
>>> >
>>> > lug 24, 2017 12:00:56 PM org.apache.bookkeeper.bookie.EntryLogger
>>> > flushRotatedLogs
>>> > INFO: Synced entry logger 0 to disk
>>> >
>>> > I am now investigating why after the first "flushRotatedLogs" bookie is
>>> > slowing down
>>> >
>>> >
>>> Thanks for the update. Eager to learn what is the culprit.
>>>
>>> JV
>>>
>>>
>>> >
>>> > Enrico
>>> >
>>> >
>>> >
>>> >
>>> > >
>>> > > Enrico
>>> > >
>>> > >
>>> > >
>>> > >>
>>> > >>
>>> > >>
>>> > >> - Sijie
>>> > >>
>>> > >> On Thu, Jul 20, 2017 at 4:49 AM, Enrico Olivelli <
>>> eolive...@gmail.com>
>>> > >> wrote:
>>> > >>
>>> > >> > Kishore, do you have news?
>>> > >> >
>>> > >> > Il ven 14 lug 2017, 09:05 Enrico Olivelli 
>>> ha
>>> > >> > scritto:
>>> > >> >
>>> > >> > > At the meeting we told the Kishore will perform some benchmarks
>>> on
>>> > his
>>> > >> > > side.
>>> > >> > > He will take a look at my code, and we are going to share the
>>> > results.
>>> > >> > > Maybe it will be possible to share the results of benchmarks
>>> done
>>> > from
>>> > >> > > Kishore at Salesforce too.
>>> > >> > >
>>> > >> > > The primary goal is to understand the differences between 4.4
>>> and
>>> > 4.5,
>>> > >> > for
>>> > >> > > instance if we there is a need to change JVM/BK configuration in
>>> > >> order to
>>> > >> > > make 4.5 perform as 4.4.
>>> > >> > >
>>> > >> > > @Sijie I hope I have answered your questions.
>>> > >> > >
>>> > >> > >
>>> > >> > > -- Enrico
>>> > >> > >
>>> > >> > >
>>> > >> > > 2017-07-13 9:29 GMT+02:00 Enrico Olivelli >> >:
>>> > >> > >
>>> > >> > >>
>>> > >> > >>
>>> > >> > >> 2017-07-13 4:11 GMT+02:00 Sijie Guo :
>>> > >> > >>
>>> > >> > >>> On Wed, Jul 12, 2017 at 10:35 PM, Enrico 

Re: BookKeeper 4.5.0 performance regression ?

[Enrico Olivelli]

I noticed that the "performance" drop in my bench happens contextually to
the opening of several "RandomAccessFile", to .dx files.

In my bench I continue to perform writes and after some time the overall
performance (latency and throughtput) "degrade"
while the bench is running I see that the overall number of open files
(with "lsof") and the number of open RandomAccessFile (using YourKit
profile) continue to grow.

The mechanics in FileInfo are quite complex

can this be a clue on my problem ?

This is the code of the bench
https://github.com/eolivelli/bookkeepers-benchs/blob/master/src/test/java/BookKeeperWriteSynchClientsTest.java

just clone from GitHub and run the test
on my laptop it starts with 80,0 MB/s throughput and when the "slow down"
occours it drops to 9 MB/s

Any suggestion ?

-- Enrico


2017-07-24 22:31 GMT+02:00 Enrico Olivelli :

>
>
> Il lun 24 lug 2017, 19:54 Venkateswara Rao Jujjuri  ha
> scritto:
>
>> On Mon, Jul 24, 2017 at 3:06 AM, Enrico Olivelli 
>> wrote:
>>
>> > 2017-07-21 20:37 GMT+02:00 Enrico Olivelli :
>> >
>> > >
>> > >
>> > > Il ven 21 lug 2017, 20:32 Sijie Guo  ha scritto:
>> > >
>> > >> As the discussion in a separate thread, it might be making sense to
>> > check
>> > >> what is the difference between using pooled allocator and unpooled
>> > >> allocator using v3 protocol. Also considering comparing using heap
>> > buffer
>> > >> and direct buffer as well.
>> > >>
>> > >> I am suspecting this might contribute latency.
>> > >>
>> > >
>> > > Yep, I am looking in this direction too.
>> > > I see that many frequent writes lead to an huge use of non heap
>> memory,
>> > > even bounding the JVM with MaxDirectMemory with max 1GB all, the 12GB
>> of
>> > my
>> > > laptop blow away during the run of my benchmark.
>> > > I suspect it is something in direct memory or something in SO caches.
>> > > I am not very skilled in SO linux memory diagnostics
>> > >
>> >
>> >
>> > I wrote a new "write intensive" benchmark, and the only thing I have
>> > noticed is that Linux is using as much RAM as possible for disk caches,
>> > this is the expected behavior on Linux.
>> >
>>
>> Yes, and this is good behavior. Why keep something unused?
>>
>
> Yes this is why linux is better then other OSs, like Windows.
>
>>
>>
>> > This is not memory allocated to the process itself.
>> > There is no difference from 4.4 and 4.5 from this aspect.
>> >
>> > I have tried the journalRemoveFromPageCachebut it brings no improvement.
>> >
>> > I did some tests as suggested by Flavio, separating the client and the
>> > bookie (even on different machines). I can say there is no "leak" nor on
>> > client side neither on bookie side.
>> >
>> >
>> > finally during my benchmarks I noticed that the real "performance drop"
>> > happens after this lines of log
>> >
>> > lug 24, 2017 12:00:56 PM org.apache.bookkeeper.bookie.EntryLogger
>> > flushRotatedLogs
>> > INFO: Synced entry logger 0 to disk
>> >
>> > I am now investigating why after the first "flushRotatedLogs" bookie is
>> > slowing down
>> >
>> >
>> Thanks for the update. Eager to learn what is the culprit.
>>
>> JV
>>
>>
>> >
>> > Enrico
>> >
>> >
>> >
>> >
>> > >
>> > > Enrico
>> > >
>> > >
>> > >
>> > >>
>> > >>
>> > >>
>> > >> - Sijie
>> > >>
>> > >> On Thu, Jul 20, 2017 at 4:49 AM, Enrico Olivelli <
>> eolive...@gmail.com>
>> > >> wrote:
>> > >>
>> > >> > Kishore, do you have news?
>> > >> >
>> > >> > Il ven 14 lug 2017, 09:05 Enrico Olivelli  ha
>> > >> > scritto:
>> > >> >
>> > >> > > At the meeting we told the Kishore will perform some benchmarks
>> on
>> > his
>> > >> > > side.
>> > >> > > He will take a look at my code, and we are going to share the
>> > results.
>> > >> > > Maybe it will be possible to share the results of benchmarks done
>> > from
>> > >> > > Kishore at Salesforce too.
>> > >> > >
>> > >> > > The primary goal is to understand the differences between 4.4 and
>> > 4.5,
>> > >> > for
>> > >> > > instance if we there is a need to change JVM/BK configuration in
>> > >> order to
>> > >> > > make 4.5 perform as 4.4.
>> > >> > >
>> > >> > > @Sijie I hope I have answered your questions.
>> > >> > >
>> > >> > >
>> > >> > > -- Enrico
>> > >> > >
>> > >> > >
>> > >> > > 2017-07-13 9:29 GMT+02:00 Enrico Olivelli :
>> > >> > >
>> > >> > >>
>> > >> > >>
>> > >> > >> 2017-07-13 4:11 GMT+02:00 Sijie Guo :
>> > >> > >>
>> > >> > >>> On Wed, Jul 12, 2017 at 10:35 PM, Enrico Olivelli <
>> > >> eolive...@gmail.com
>> > >> > >
>> > >> > >>> wrote:
>> > >> > >>>
>> > >> > >>> > Sijie, JV, just a recap my point of view:
>> > >> > >>> > - considering latency = "time for asynchAddEntry to complete"
>> > >> > >>> > - there is a some difference from 4.4 and 4.5 in the usage of
>> > >> memory,
>> > >> > >>> but
>> > >> > >>> > no so clear
>> > >> > >>> > - the type of GC (parallel vs G1) does not 

Re: EnsemblePlacementPolicy Public API breaking changes

[Enrico Olivelli]

2017-07-24 22:54 GMT+02:00 Sijie Guo :

> On Mon, Jul 24, 2017 at 1:29 PM, Enrico Olivelli 
> wrote:
>
> > Il lun 24 lug 2017, 19:39 Sijie Guo  ha scritto:
> >
> > > On Mon, Jul 24, 2017 at 5:22 AM, Enrico Olivelli 
> > > wrote:
> > >
> > > > Hi,
> > > > we are going to release 4.5 soon, in this release we changed
> > > > EnsemblePlacementPolicy interface.
> > > >
> > > > It you are using a "custom" EnsemblePlacementPolicy in 4.4 and you
> just
> > > > switch the bookeeeper-server JAR on the classpath your application
> > won't
> > > > run.
> > > >
> > > > I already got that problem and I needed to implement some "support
> 4.5"
> > > > option in my projects which essentially tells "do not use a custom
> > > policy",
> > > > this is very bad because there is no way to apply the custom rules
> > > required
> > > > by the project.
> > > >
> > >
> > > Do you mean the new methods introduced in placement policy? Or methods
> > that
> > > whose signatures are changed?
> > >
> >
> > The new signatures
> >
> > >
> > > I believe the new methods 'reorderSequence' is disabled by default
> unless
> > > you enable it explicitly.
> > >
> > > For methods that whose signatures were changed, we can add the old
> > methods
> > > back and create overrides to keep the binary backward compatible.
> > >
> >
> > I don't know if this cam work. I have already tried. I will double check.
> > Anyway it will be a bit messy
> >
> > >
> > > However I am not sure if this works because the placement policy is
> > > instantiated and loaded by reflection. Typically when you upgrade a
> > > version, you have to compile it first, no?
> > >
> >
> > I have several libraries which use bk and they are built on 4.4 and they
> > are working together in the same classpath.
> > For instance now I am going to change one of them in order to leverage a
> > new 4.5  feature, like readUnconfirmedEntries just as an example, so I
> need
> > to switch to 4.5 on the classpath but the other projects won't run.
> >
>
> Are you talking about mixing 4.4 and 4.5 together?
>

Yes, as for most widely used libraries (like ZooKeeper) it is common to put
a "new version" or the library on a application compiled for a previous
version.
I see that sometimes it is not possible, and for this reason we use 'major
versions' vs 'minor versions'.
When possible I think it is best not to introduce such breaking changes.





>
>
> > For the server side this is not a problem but on the client it is a real
> > production problem.
> >
>
> Can you clarify more specifically on this? Not very sure I understand your
> problem and what are you going to achieve.
>
>
Usually you have full control on bookies, but it is not always possible to
have full control of applications assembler from different components.
Example:
- I have Lib 1 v1 which is compiled with BK 4.4
- I have Lib 2 v1 which is compiled with BK 4.4
- I have App which depends on Lib1 + Lib2
- Now Lib 2 v2 need BK 4.5 for a fresh new feature
- New version of the App v2 will depend on Lib 1 v1 + Lib 2 v2 and will
need to include BK 4.5, but this won't work
Assemblers for App v2 do not have control on Lib1 and Lib2

For instance I got into this trouble while integrating my new project which
uses readUnconfirmedEntries inside an App which is compiled for BK 4.4 and
uses a custom placement policy.

So, now that we are breaking things I would like to break them in a way
that in the future will be simpler to handle

for instance:

public interface EnsemblePlacementPolicy {


public EnsemblePlacementPolicy initialize(ClientEnvironment env);

public void uninitalize();

public Set
onClusterChanged(Set writableBookies,

Set readOnlyBookies);

public List newEnsemble(LedgerSpecs ledgerSpecs,
Set excludeBookies) throws BKNotEnoughBookiesException;


public BookieSocketAddress replaceBookie(LedgerSpecs ledgerSpecs,
Collection currentEnsemble,
BookieSocketAddress bookieToReplace, Set
excludeBookies) throws BKNotEnoughBookiesException;

public List reorderReadSequence(List
ensemble,
 List writeSet,
Map bookieFailureHistory);


public List reorderReadLACSequence(List
ensemble,
List writeSet,
Map bookieFailureHistory);

default public void updateBookieInfo(Map bookieInfoMap) {
}
}


where ClientEnvironment and LedgerSpecs contain all of the data which need
to be passed to the policy.
We can do even more better and group the other parameters

we can evaluate to switch to an abstract class, or provide an official
"Adapter", but DefaultEnsemblePlacementPolicy is already a good "base"



>
>
> > I am ok with changing the API, I was the first to ask to change it in 4.5
> > in order to access custom metadata, but when we do such things I would
> 

[GitHub] eolivelli opened a new pull request #291: Issue-290 Add checkstyle:checkstyle to bk-merge script

[git]

eolivelli opened a new pull request #291: Issue-290 Add checkstyle:checkstyle 
to bk-merge script
URL: https://github.com/apache/bookkeeper/pull/291
 
 
   Add checkstyle:checkstyle to the merge script.
   This will be useful in order to check the patch before every commit on the 
repo
 

This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] eolivelli opened a new issue #290: Add checkstyle:checkstyle to bk-merge script

[git]

eolivelli opened a new issue #290: Add checkstyle:checkstyle to bk-merge script
URL: https://github.com/apache/bookkeeper/issues/290
 
 
   **FEATURE REQUEST**
   
   1. Please describe the feature you are requesting.
   I want an utility to run "checkstyle" before merging patches
   
   2. Indicate the importance of this issue to you (blocker, must-have, 
should-have, nice-to-have). Are you currently using any workarounds to address 
this issue?
   should-have
   
 

This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] eolivelli closed pull request #285: Issue 247: Missing JavaDoc for classes

[git]

eolivelli closed pull request #285: Issue 247: Missing JavaDoc for classes
URL: https://github.com/apache/bookkeeper/pull/285
 
 
   
 

This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] eolivelli closed issue #247: Missing JavaDoc for classes

[git]

eolivelli closed issue #247: Missing JavaDoc for classes
URL: https://github.com/apache/bookkeeper/issues/247
 
 
   
 

This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] eolivelli closed pull request #286: BOOKKEEPER-1044: Entrylogger is not readding rolled logs back to the logChannelsToFlush list when exception happens while trying to flush rolled logs

[git]

eolivelli closed pull request #286: BOOKKEEPER-1044: Entrylogger is not 
readding rolled logs back to the logChannelsToFlush list when exception happens 
while trying to flush rolled logs
URL: https://github.com/apache/bookkeeper/pull/286
 
 
   
 

This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] eolivelli commented on a change in pull request #276: Issue-271 LedgerHandle#readEntries leaks ByteBufs

[git]

eolivelli commented on a change in pull request #276: Issue-271 
LedgerHandle#readEntries leaks ByteBufs
URL: https://github.com/apache/bookkeeper/pull/276#discussion_r129248338
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/ClientConfiguration.java
 ##
 @@ -114,6 +116,10 @@
 protected final static String ENABLE_BOOKIE_FAILURE_TRACKING = 
"enableBookieFailureTracking";
 protected final static String BOOKIE_FAILURE_HISTORY_EXPIRATION_MS = 
"bookieFailureHistoryExpirationMSec";
 
+// Netty
+protected final static String NETTY_USE_POOLED_BUFFERS = 
"nettyUsePooledBuffers";
+protected final static boolean DEFAULT_NETTY_USE_POOLED_BUFFERS = true;
 
 Review comment:
   ok, done
 

This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] eolivelli commented on a change in pull request #276: Issue-271 LedgerHandle#readEntries leaks ByteBufs

[git]

eolivelli commented on a change in pull request #276: Issue-271 
LedgerHandle#readEntries leaks ByteBufs
URL: https://github.com/apache/bookkeeper/pull/276#discussion_r129248305
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/ClientConfiguration.java
 ##
 @@ -114,6 +116,10 @@
 protected final static String ENABLE_BOOKIE_FAILURE_TRACKING = 
"enableBookieFailureTracking";
 protected final static String BOOKIE_FAILURE_HISTORY_EXPIRATION_MS = 
"bookieFailureHistoryExpirationMSec";
 
+// Netty
 
 Review comment:
   done
 

This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] eolivelli commented on a change in pull request #276: Issue-271 LedgerHandle#readEntries leaks ByteBufs

[git]

eolivelli commented on a change in pull request #276: Issue-271 
LedgerHandle#readEntries leaks ByteBufs
URL: https://github.com/apache/bookkeeper/pull/276#discussion_r129248134
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerEntry.java
 ##
 @@ -53,22 +54,49 @@ public long getLength() {
 return length;
 }
 
+/**
+ * Returns the content of the entry.
+ * This method can be called only once.
+ * While using v2 wire protocol this method will automatically release the 
internal ByteBuf
+ * @return
+ */
 public byte[] getEntry() {
+if (data == null) {
+throw new IllegalStateException("entry content can be accessed 
only once");
+}
 byte[] entry = new byte[data.readableBytes()];
 data.readBytes(entry);
 data.release();
+data = null;
 return entry;
 }
 
+/**
+ * Returns the content of the entry.
+ * This method can be called only once.
+ * While using v2 wire protocol this method will automatically release the 
internal ByteBuf when calling the close
+ * method of the returned InputStream
+ *
+ * @return an InputStream which gives access to the content of the entry
+ */
 public InputStream getEntryInputStream() {
-return new ByteBufInputStream(data);
+if (data == null) {
 
 Review comment:
   same as above
 

This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] eolivelli commented on a change in pull request #276: Issue-271 LedgerHandle#readEntries leaks ByteBufs

[git]

eolivelli commented on a change in pull request #276: Issue-271 
LedgerHandle#readEntries leaks ByteBufs
URL: https://github.com/apache/bookkeeper/pull/276#discussion_r129248110
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerEntry.java
 ##
 @@ -53,22 +54,49 @@ public long getLength() {
 return length;
 }
 
+/**
+ * Returns the content of the entry.
+ * This method can be called only once.
+ * While using v2 wire protocol this method will automatically release the 
internal ByteBuf
+ * @return
+ */
 public byte[] getEntry() {
+if (data == null) {
 
 Review comment:
   @sijie the first time getEntry is called we will set data to null, this way 
the client cannot re-use the buffer anymore.
   On an eventual "second call" we will throw an IllegalStateException which 
explains the error.
   if I drop the 'if' the user will get a NullPointerException, but this is not 
very clear and IMHO it is bad practice.
   
   Unfortunately the ByteBuf can be used only once, because we have no clue to 
reset the original start position for the entry
 

This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] jiazhai opened a new issue #289: Docker image: make docker image build based on master latest code

[git]

jiazhai opened a new issue #289: Docker image: make docker image build based on 
master latest code 
URL: https://github.com/apache/bookkeeper/issues/289
 
 
   In the first docker image PR (#197), there is a suggestion to make docker 
image build based on master latest code 
 

This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

Re: [DISCUSS] Slack Channel for BookKeeper

[Enrico Olivelli]

Awesome +1

Il mar 25 lug 2017, 05:39 Dustin Castor  ha
scritto:

> Agreed! Or an IRC.
>
> On Monday, July 24, 2017, 6:50:22 PM PDT, Jia Zhai 
> wrote:
>
>  It is great to have a slack channel. It make things more effective and
> smooth.
>
> On Tue, Jul 25, 2017 at 8:11 AM, Sijie Guo  wrote:
>
> > Hi all,
> >
> > What do you guys all think about having a dedicated slack channel for
> > informal discussion for the community? There are a handful of Apache
> > projects are doing that already, there are also ways to have a bot that
> > sends daily digest of the conversation to the mailing lists (to keep the
> > records in ASF infrastructure).
> >
> > As the followup steps for merging DL into BookKeeper, we are transferring
> > the DL slack channel to BookKeeper PMC. We can just make it a BK slack
> > channel, and have different channels under it for different discussions.
> >
> > Thoughts?
> >
> > - Sijie
> >

-- 


-- Enrico Olivelli

[jira] [Commented] (BOOKKEEPER-1016) Create documentation for JAAS and Kerberos configuration

[Enrico Olivelli (JIRA)]

[ 
https://issues.apache.org/jira/browse/BOOKKEEPER-1016?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16099581#comment-16099581
 ] 

Enrico Olivelli commented on BOOKKEEPER-1016:
-

I am sorry. This week I am not sure to have time. If you want to pick it up for 
me it is no problem.
Some info and the jaas.conf instructions are already in the security doc.


> Create documentation for JAAS and Kerberos configuration
> 
>
> Key: BOOKKEEPER-1016
> URL: https://issues.apache.org/jira/browse/BOOKKEEPER-1016
> Project: Bookkeeper
>  Issue Type: Sub-task
>  Components: bookkeeper-client, bookkeeper-server, Documentation
>Affects Versions: 4.5.0
>Reporter: Enrico Olivelli
>Assignee: Enrico Olivelli
>Priority: Blocker
> Fix For: 4.5.0
>
>
> We need to document how to setup a secure BookKeeper cluster, see 
> BOOKEEPER-391



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[GitHub] eolivelli commented on issue #183: BOOKKEEPER-588 SSL Support for Bookkeeper

[git]

eolivelli commented on issue #183: BOOKKEEPER-588 SSL Support for Bookkeeper
URL: https://github.com/apache/bookkeeper/pull/183#issuecomment-317644019
 
 
   From my point of view it can be deferred to 4.6. I needed zk acls and sasl 
and all of these features are in
 

This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services