Re: Question regarding Synchronization in InterleavedLedgerStorage

2017-07-14 Thread Sijie Guo 
On Sat, Jul 15, 2017 at 8:06 AM, Charan Reddy G 
wrote:

> Hey,
>
> In InterleavedLedgerStorage, since the initial version of it (
> https://github.com/apache/bookkeeper/commit/4a94ce1d8184f5f38def015d80777a
> 8113b96690 and https://github.com/apache/bookkeeper/commit/
> d175ada58dcaf78f0a70b0ebebf489255ae67b5f), addEntry and processEntry
> methods are synchronized. If it is synchronized then I dont get what is the
> point in having 'writeThreadPool' in BookieRequestProcessor, if anyhow they
> are going to be executed sequentially because of synchronized addEntry
> method in InterleavedLedgerStorage.
>

When InterleavedLedgerStore is used in the context of SortedLedgerStore,
the addEntry and processEntry are only called when flushing happened. The
flushing happens in background thread, which is effectively running
sequentially. But adding to the memtable happens concurrently.

The reason of having 'writeThreadPool' is more on separating writes and
reads into different thread pools. so writes will not be affected by reads.
In the context of SortedLedgerStore, the 'writeThreadPool' adds the
concurrency.


>
> If we look at the implementation of addEntry and processEntry method,
> 'somethingWritten' can be made threadsafe by using AtomicBoolean,
> ledgerCache.updateLastAddConfirmed and entryLogger.addEntry methods are
> inherently threadsafe.
>
> I'm not sure about semantics of ledgerCache.putEntryOffset method here.
> I'm not confident enough to say if LedgerCacheImpl and IndexInMemPageMgr
> (and probably IndexPersistenceMgr) are thread-safe classes.
>

LedgerCacheImpl and IndexInMemPageMgr are thread-safe classes. You can
confirm this from SortedLedgerStorage.


>
> As far as I understood, if ledgerCache.putEntryOffset is thread safe, then
> I dont see the need of synchronization for those methods. In any case, if
> they are not thread-safe can you please say why it is not thread-safe and
> how we can do more granular synchronization at LedgerCacheImpl level, so
> that we can remove the need of synchrnoization at InterleavedLedgerStorage
> level.
>

I don't see any reason why we can't remove the synchronization.


>
> I'm currently working on Multiple Entrylogs - https://issues.apache.org/
> jira/browse/BOOKKEEPER-1041.
>

I am wondering if your multiple-entrylogs approach is making things
complicated. I have been thinking there can be a simpler approach achieving
the same goal: for example, having a ledger storage comprised of N
interleaved/sorted ledger storages, which they share same LedgerCache, but
having different memtables (for sortedledgerstore) and different entry log
files.


> To reap the benefits of multipleentrylogs feature from performance
> perspective, this synchrnoization should be taken care or atleast bring it
> down to more granular synchronization (if possible).
>
> @Override
> synchronized public long addEntry(ByteBuffer entry) throws IOException
> {
> long ledgerId = entry.getLong();
> long entryId = entry.getLong();
> long lac = entry.getLong();
> entry.rewind();
> processEntry(ledgerId, entryId, entry);
> ledgerCache.updateLastAddConfirmed(ledgerId, lac);
> return entryId;
> }
>
> synchronized protected void processEntry(long ledgerId, long entryId,
> ByteBuffer entry, boolean rollLog)
> throws IOException {
> somethingWritten = true;
> long pos = entryLogger.addEntry(ledgerId, entry, rollLog);
> ledgerCache.putEntryOffset(ledgerId, entryId, pos);
> }
>
> Thanks,
> Charan
>

Question regarding Synchronization in InterleavedLedgerStorage

2017-07-14 Thread Charan Reddy G 
Hey,

In InterleavedLedgerStorage, since the initial version of it (
https://github.com/apache/bookkeeper/commit/4a94ce1d8184f5f38def015d80777a8113b96690
and
https://github.com/apache/bookkeeper/commit/d175ada58dcaf78f0a70b0ebebf489255ae67b5f),
addEntry and processEntry methods are synchronized. If it is synchronized
then I dont get what is the point in having 'writeThreadPool' in
BookieRequestProcessor, if anyhow they are going to be executed
sequentially because of synchronized addEntry method in
InterleavedLedgerStorage.

If we look at the implementation of addEntry and processEntry method,
'somethingWritten' can be made threadsafe by using AtomicBoolean,
ledgerCache.updateLastAddConfirmed and entryLogger.addEntry methods are
inherently threadsafe.

I'm not sure about semantics of ledgerCache.putEntryOffset method here. I'm
not confident enough to say if LedgerCacheImpl and IndexInMemPageMgr (and
probably IndexPersistenceMgr) are thread-safe classes.

As far as I understood, if ledgerCache.putEntryOffset is thread safe, then
I dont see the need of synchronization for those methods. In any case, if
they are not thread-safe can you please say why it is not thread-safe and
how we can do more granular synchronization at LedgerCacheImpl level, so
that we can remove the need of synchrnoization at InterleavedLedgerStorage
level.

I'm currently working on Multiple Entrylogs -
https://issues.apache.org/jira/browse/BOOKKEEPER-1041. To reap the benefits
of multipleentrylogs feature from performance perspective, this
synchrnoization should be taken care or atleast bring it down to more
granular synchronization (if possible).

@Override
synchronized public long addEntry(ByteBuffer entry) throws IOException {
long ledgerId = entry.getLong();
long entryId = entry.getLong();
long lac = entry.getLong();
entry.rewind();
processEntry(ledgerId, entryId, entry);
ledgerCache.updateLastAddConfirmed(ledgerId, lac);
return entryId;
}

synchronized protected void processEntry(long ledgerId, long entryId,
ByteBuffer entry, boolean rollLog)
throws IOException {
somethingWritten = true;
long pos = entryLogger.addEntry(ledgerId, entry, rollLog);
ledgerCache.putEntryOffset(ledgerId, entryId, pos);
}

Thanks,
Charan

[GitHub] sijie commented on issue #205: Issue 208: Improve ledger fence logic

2017-07-14 Thread git 
sijie commented on issue #205: Issue 208: Improve ledger fence logic
URL: https://github.com/apache/bookkeeper/pull/205#issuecomment-315493350
 
 
   @eolivelli I am currently traveling. I will spend some time on rebasing my 
changes ASAP when I have a large chunk of time.
 

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 #241: Merge script does not deal well with non-ascii names of reviewers

2017-07-14 Thread git 
sijie commented on issue #241: Merge script does not deal well with non-ascii 
names of reviewers
URL: https://github.com/apache/bookkeeper/issues/241#issuecomment-315492989
 
 
   @eolivelli @jiazhai - it might be worth opening an issue to make the script 
handle non-ascii strings extracted from user/committer info.
 

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 #233: WIP - Issue 232: Add code-coverage report

2017-07-14 Thread git 
sijie commented on issue #233: WIP - Issue 232: Add code-coverage report
URL: https://github.com/apache/bookkeeper/pull/233#issuecomment-315492650
 
 
   LGTM +1
 

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 #244: Issue#243 - asyncAddEntry fails with NPE with LedgerHandlerAdv

2017-07-14 Thread git 
sijie commented on a change in pull request #244: Issue#243 - asyncAddEntry 
fails with NPE with LedgerHandlerAdv
URL: https://github.com/apache/bookkeeper/pull/244#discussion_r127568518
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerHandle.java
 ##
 @@ -720,8 +720,7 @@ public void asyncAddEntry(final byte[] data, final 
AddCallback cb,
  * @param ctx
  *some control object
  */
-public void asyncAddEntry(final long entryId, final byte[] data, final 
AddCallback cb, final Object ctx)
-throws BKException {
+public void asyncAddEntry(final long entryId, final byte[] data, final 
AddCallback cb, final Object ctx) {
 
 Review comment:
   since we are touching this function now, I'd like to also raise one 
discussing about the semantic here. Currently it is totally the application's 
responsibility on managing the semantic on using this API. it would be good to 
add some constraints on this API:
   
   - if entryId is negative  (the fix @eolivelli tried to fix)
   - if entryId is smaller than LAC, that means this entry is a duplicated entry
   
   in both cases, we should fail the addEntry with exceptions.
 

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 #244: Issue#243 - asyncAddEntry fails with NPE with LedgerHandlerAdv

2017-07-14 Thread git 
sijie commented on a change in pull request #244: Issue#243 - asyncAddEntry 
fails with NPE with LedgerHandlerAdv
URL: https://github.com/apache/bookkeeper/pull/244#discussion_r127567837
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerHandleAdv.java
 ##
 @@ -165,6 +165,11 @@ public void asyncAddEntry(final long entryId, final 
byte[] data, final int offse
  */
 @Override
 protected void doAsyncAddEntry(final PendingAddOp op, final ByteBuf data, 
final AddCallback cb, final Object ctx) {
+if (op.entryId < 0 ) {
 
 Review comment:
   when will this happen? I think this change volatiles the semantic of 
LedgerHandleAdv#asyncAddEntry. if the entry id is negative, we should reject 
this add with illegal arguments. 
 

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 #243: asyncAddEntry fails with NPE with LedgerHandler created with createLedgerAdv

2017-07-14 Thread git 
sijie commented on issue #243: asyncAddEntry fails with NPE with LedgerHandler 
created with createLedgerAdv
URL: https://github.com/apache/bookkeeper/issues/243#issuecomment-315491619
 
 
   @eolivelli can you share the code that cause the NPE? 
 

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: Git protected branches

2017-07-14 Thread Sijie Guo 
I don't think INFRA extends such admin privileges to projects. I think this
is how Gitbox is working. But if we want PMC/projects want to have more
privileges, that might require broader discussions in the ASF.

- Sijie

On Fri, Jul 14, 2017 at 3:13 PM, Enrico Olivelli 
wrote:

> Yes Flavio,
> it is a pity that we cannot ever see in readonly mode the actual
> configuration.
> If you look at the issue, the operator attached a screenshot in order to
> let me see the actual configuration
>
> Maybe as PMC you or at least Sijie can ask for more privileges
>
> -- Enrico
>
> 2017-07-14 9:10 GMT+02:00 Flavio Junqueira :
>
> > It's kind of odd that we have to go through infra to do this. Such a
> > configuration feels like should be under the control of committers or at
> > least the PMC.
> >
> > Thanks for doing it in any case, Enrico.
> >
> > -Flavio
> >
> > > On 13 Jul 2017, at 01:55, Jia Zhai  wrote:
> > >
> > > 
> > >
> > > On Thu, Jul 13, 2017 at 5:34 AM, Venkateswara Rao Jujjuri <
> > jujj...@gmail.com
> > >> wrote:
> > >
> > >> Awesome. !!
> > >>
> > >> On Tue, Jul 11, 2017 at 6:06 AM, Enrico Olivelli  >
> > >> wrote:
> > >>
> > >>> The ticket has been closed.
> > >>> The protection is on
> > >>>
> > >>> -- Enrico
> > >>>
> > >>> 2017-07-07 11:22 GMT+02:00 Enrico Olivelli :
> > >>>
> > 
> > 
> >  2017-07-07 1:51 GMT+02:00 Sijie Guo :
> > 
> > > +1 for disable force push to master
> > >
> > 
> > 
> >  this is the issue:
> >  https://issues.apache.org/jira/browse/INFRA-14535
> > 
> >  I will send updates
> > 
> >  - Enrico
> > 
> > 
> > >
> > > On Thu, Jul 6, 2017 at 1:16 PM, Enrico Olivelli <
> eolive...@gmail.com
> > >
> > > wrote:
> > >
> > >> Other thoughts?
> > >> If there is no objection I would like to enable the force push
> > >> block.
> > > Next
> > >> week
> > >>
> > >> Enrico
> > >>
> > >> Il mar 4 lug 2017, 10:28 Enrico Olivelli  ha
> > > scritto:
> > >>
> > >>> 2017-07-04 5:51 GMT+02:00 Jia Zhai :
> >  Agree to have them protected.
> >  Currently, seems these branches are not protected, at least
> > >> master
> > >> branch
> >  is not. The merge button is still available even "Jenkins: Maven
> > > clean
> >  install — Build finished." in some of PRs
> >  
> >  We may first need to make the test stable.
> > >>>
> > >>> I think that is it enough to only protect against "force push",
> > >>> which
> > >>> makes impossible for anyone to 'rewrite' the "public history" of
> > >> the
> > >>> project
> > >>> Other checks are performed by QA test and by the committer who
> > >> takes
> > >>> responsibility for pushing the patch in the repo
> > >>>
> > >>>
> > >>> -- Enrico
> > >>>
> > >>>
> > 
> >  On Tue, Jul 4, 2017 at 5:10 AM, Enrico Olivelli <
> > > eolive...@gmail.com>
> > >>> wrote:
> > 
> > > Hi,
> > > I don't know current configuration but I would like to ensure
> > >>> that
> > > our
> > > release branches, that is master and branch-xx are 'protected'.
> > > I mean at least that 'force push' must be forbidden to
> > >> everyone.
> > >
> > > In github it is simple to enable such protection, see
> > > https://help.github.com/articles/about-protected-branches/
> > >
> > > I don't want to try btyy performing a force push. Maybe we can
> > >>> ask
> > >> infra
> > > about current configuration
> > >
> > > Thoughts?
> > >
> > > Enrico
> > > --
> > >
> > >
> > > -- Enrico Olivelli
> > >
> > >>>
> > >> --
> > >>
> > >>
> > >> -- Enrico Olivelli
> > >>
> > >
> > 
> > 
> > >>>
> > >>
> > >>
> > >>
> > >> --
> > >> Jvrao
> > >> ---
> > >> First they ignore you, then they laugh at you, then they fight you,
> then
> > >> you win. - Mahatma Gandhi
> > >>
> >
> >
>

[GitHub] eolivelli opened a new pull request #244: Issue#243 - asyncAddEntry fails with NPE with LedgerHandlerAdv

2017-07-14 Thread git 
eolivelli opened a new pull request #244: Issue#243 - asyncAddEntry fails with 
NPE with LedgerHandlerAdv
URL: https://github.com/apache/bookkeeper/pull/244
 
 
   Fix asyncAddEntry on LedgerHandleAdv and clean up the asyncAddEntry API, 
drops BKException which is never thrown in asynch functions
 

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 #243: asyncAddEntry fails with NPE with LedgerHandler created with createLedgerAdv

2017-07-14 Thread git 
eolivelli commented on issue #243: asyncAddEntry fails with NPE with 
LedgerHandler created with createLedgerAdv
URL: https://github.com/apache/bookkeeper/issues/243#issuecomment-315383045
 
 
   The signature of asyncAddEntry is wrong, it does not throw BKException as it 
is an async function
   ```
   public void asyncAddEntry(final long entryId, final byte[] data, final int 
offset, final int length,
   final AddCallback cb, final Object ctx) throws BKException
   ```
   
   Maybe we can fix this error 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

[GitHub] eolivelli opened a new issue #243: asyncAddEntry fails with NPE with LedgerHandler created with createLedgerAdv

2017-07-14 Thread git 
eolivelli opened a new issue #243: asyncAddEntry fails with NPE with 
LedgerHandler created with createLedgerAdv
URL: https://github.com/apache/bookkeeper/issues/243
 
 
   It turns out on 4.5 that "asyncAddEntry" fails with NPE with LedgerHandler 
created with createLedgerAdv
   
   ```
   java.lang.NullPointerException
at 
org.apache.bookkeeper.client.PendingAddOp.initiate(PendingAddOp.java:195)
at 
org.apache.bookkeeper.client.LedgerHandleAdv$2.safeRun(LedgerHandleAdv.java:216)
   ```
 

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

Jenkins build is back to normal : bookkeeper-master #1815

2017-07-14 Thread Apache Jenkins Server 
See

[GitHub] eolivelli commented on issue #232: Add code-coverage report

2017-07-14 Thread git 
eolivelli commented on issue #232: Add code-coverage report
URL: https://github.com/apache/bookkeeper/issues/232#issuecomment-315330556
 
 
   @sijie  @fpj  is using CodeCov which is well integrated with PRs, I will 
take a look too
   
   It seems that it requires Cobertura too.
   Both Coveralls and Codecov do not require "tokens" if the build is run from 
Travis-CI
   I think that the better approach is to run from Travis
 

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 #232: Add code-coverage report

2017-07-14 Thread git 
eolivelli commented on issue #232: Add code-coverage report
URL: https://github.com/apache/bookkeeper/issues/232#issuecomment-315330556
 
 
   @sijie  @fpj  is using CodeCov which is well integrated with PRs, I will 
take a look too
   
   It seems that it requires Cobertura too.
   Both Coveralls and Codecov do not require "tokens" if the build is run from 
Travis-CI
   I think that the better approch is to run from Travis
 

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 #241: Merge script does not deal well with non-ascii names of reviewers

2017-07-14 Thread git 
eolivelli closed issue #241: Merge script does not deal well with non-ascii 
names of reviewers
URL: https://github.com/apache/bookkeeper/issues/241
 
 
   
 

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 #241: Merge script does not deal well with non-ascii names of reviewers

2017-07-14 Thread git 
eolivelli commented on issue #241: Merge script does not deal well with 
non-ascii names of reviewers
URL: https://github.com/apache/bookkeeper/issues/241#issuecomment-315328854
 
 
   closed by merging #242 
 

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 #242: Issue 241: handle non-ascii chars in username in merge script

2017-07-14 Thread git 
eolivelli closed pull request #242: Issue 241: handle non-ascii chars in 
username in merge script
URL: https://github.com/apache/bookkeeper/pull/242
 
 
   
 

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 #241: Merge script does not deal well with non-ascii names of reviewers

2017-07-14 Thread git 
eolivelli commented on issue #241: Merge script does not deal well with 
non-ascii names of reviewers
URL: https://github.com/apache/bookkeeper/issues/241#issuecomment-315326358
 
 
   @jiazhai I am sorry the testcase is not usefull, the bug is on reviewers and 
on on the 'author', I am loking for a good test case
 

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 #241: Merge script does not deal well with non-ascii names of reviewers

2017-07-14 Thread git 
eolivelli commented on issue #241: Merge script does not deal well with 
non-ascii names of reviewers
URL: https://github.com/apache/bookkeeper/issues/241#issuecomment-315326061
 
 
   @jiazhai  I am checking your patch. As soon as I am OK I think we can merge 
without @sijie it is ery trivial
 

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 commented on issue #241: Merge script does not deal well with non-ascii names of reviewers

2017-07-14 Thread git 
jiazhai commented on issue #241: Merge script does not deal well with non-ascii 
names of reviewers
URL: https://github.com/apache/bookkeeper/issues/241#issuecomment-315324040
 
 
   @eolivelli, thanks a lot for the info, have a try on 234 with a fix, and it 
passed:
   ```
   git:(issue_241) python dev/bk-merge-pr.py
   git rev-parse --abbrev-ref HEAD
   Which pull request would you like to merge? (e.g. 34): 234
   Commit title [Issue 230: Add Checkstyle to the build process (Part 2)]: 
   I've re-written the title as follows to match the standard format:
   Original: Issue 230: Add Checkstyle to the build process (Part 2)
   Modified: ISSUE #230: Add Checkstyle to the build process (Part 2)
   Would you like to use the modified title? (y/n): y
   Using modified title:
   ISSUE #230: Add Checkstyle to the build process (Part 2)
   
   === Pull Request #234 ===
   PR title Issue 230: Add Checkstyle to the build process (Part 2)
   Commit title ISSUE #230: Add Checkstyle to the build process (Part 2)
   Source   sigee/checkstyle2
   Target   master
   URL  https://api.github.com/repos/apache/bookkeeper/pulls/234
   
   Proceed with merging pull request #234? (y/n): y
   git fetch apache-github pull/234/head:PR_TOOL_MERGE_PR_234
   remote: Counting objects: 87, done.
   remote: Compressing objects: 100% (12/12), done.
   remote: Total 87 (delta 69), reused 87 (delta 69), pack-reused 0
   Unpacking objects: 100% (87/87), 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] jiazhai opened a new pull request #242: Issue 241: handle non-ascii chars in username in merge script

2017-07-14 Thread git 
jiazhai opened a new pull request #242: Issue 241: handle non-ascii chars in 
username in merge script
URL: https://github.com/apache/bookkeeper/pull/242
 
 
   Descriptions of the changes in this PR:
   Handle handle non-ascii chars in username in merge script.
   
   ---
   Be sure to do all of the following to help us incorporate your contribution
   quickly and easily:
   
   - [X] Make sure the PR title is formatted like:
   `: Description of pull request`
   `e.g. Issue 123: Description ...`
   `e.g. BOOKKEEPER-1234: Description ...`
   - [X] Make sure tests pass via `mvn clean apache-rat:check install 
findbugs:check`.
   - [X] Replace `` in the title with the actual 
Issue/JIRA number.
   
   ---
   
 

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 #231: Issue 230: Add Checkstyle to the build process (Part 1)

2017-07-14 Thread git 
eolivelli commented on issue #231: Issue 230: Add Checkstyle to the build 
process (Part 1)
URL: https://github.com/apache/bookkeeper/pull/231#issuecomment-315320256
 
 
   @sigee @sijie @fpj 
   we discussed about the TODOs at the meeting, there was consensus about 
creating issues for each TODO (or of groups of TODOs) and then putting links to 
each link in each TODO.
   
   so please @sigee update the patch and I think it is OK for merge
 

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 #241: Merge script does not deal well with non-ascii names of reviewers

2017-07-14 Thread git 
eolivelli commented on issue #241: Merge script does not deal well with 
non-ascii names of reviewers
URL: https://github.com/apache/bookkeeper/issues/241#issuecomment-315319819
 
 
   thank you @jiazhai 
   I think that you can play with #234, that comes from @sigee, without 
actually pushing 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] jiazhai commented on issue #241: Merge script does not deal well with non-ascii names of reviewers

2017-07-14 Thread git 
jiazhai commented on issue #241: Merge script does not deal well with non-ascii 
names of reviewers
URL: https://github.com/apache/bookkeeper/issues/241#issuecomment-315319206
 
 
   would like to have a try to handle 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] jiazhai commented on issue #240: ISSUE #239: typo in class name SpeculativeRequestExecutor

2017-07-14 Thread git 
jiazhai commented on issue #240: ISSUE #239: typo in class name 
SpeculativeRequestExecutor
URL: https://github.com/apache/bookkeeper/pull/240#issuecomment-315319050
 
 
   @eolivelli. thanks for the info. @sigee, From my view, your name certainly 
have no error, we will fix the merge script soon.  :)
 

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 a change in pull request #197: BOOKKEEPER-974: Add an official bookkeeper docker image

2017-07-14 Thread git 
caiok commented on a change in pull request #197: BOOKKEEPER-974: Add an 
official bookkeeper docker image
URL: https://github.com/apache/bookkeeper/pull/197#discussion_r127412474
 
 

 ##
 File path: docker/4.4.0-alpine/Dockerfile
 ##
 @@ -0,0 +1,59 @@
+FROM java:openjdk-8-jre-alpine
+MAINTAINER Francesco Caliumi 
+
+# Install required packages
+RUN apk add --no-cache \
+bash \
+su-exec
+
+ENV ZK_SERVERS= \
+BK_USER=bookkeeper \
+BK_PORT= \
 
 Review comment:
   @sijie @jiazhai  I remembered the reason behind this implementation. We 
provide to docker user the ability to mount configurations file at run time. 
These files will be rewritten with the env variables set at run. If we do 
provide a default for the port, in run.sh we have no way to determine if was 
passed by the user or it's the default, so it will be rewritten in each case, 
overwriting the port specified by the user in conf file. I will restore the 
BK_PORT=[blank] and BK_BUILD_PORT
 

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 a change in pull request #197: BOOKKEEPER-974: Add an official bookkeeper docker image

2017-07-14 Thread git 
caiok commented on a change in pull request #197: BOOKKEEPER-974: Add an 
official bookkeeper docker image
URL: https://github.com/apache/bookkeeper/pull/197#discussion_r127411473
 
 

 ##
 File path: docker/4.4.0-alpine/run.sh
 ##
 @@ -0,0 +1,68 @@
+#/bin/bash
+
+# -- #
+set -x -e -u
+# -- #
+
+# -- #
+# Allow the container to be started with `--user`
+if [ "$1" = 'bookkeeper' -a "$(id -u)" = '0' ]; then
+chown -R "$BK_USER" "${BK_DIR}" "${BK_JOURNAL_DIR}" "${BK_LEDGER_DIR}" 
"${BK_INDEX_DIR}"
+exec su-exec "$BK_USER" /bin/bash "$0" "$@"
+exit
+fi
+# -- #
+
+# -- #
+# Copy input config files in Bookkeeper configuration directory
+cp -vaf /conf/* ${BK_DIR}/conf || true
+chown -R "$BK_USER" ${BK_DIR}/conf
+
+# Bookkeeper setup
 
 Review comment:
   Is the first step really needed or we could use the default bk_server.conf 
and then in run phase use found BK_xxx env variables for substitute confs in 
it? Furthermore, the first step will brake the possibility to mount an already 
set configuration file (it would be entirely rewritten). 
 

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 a change in pull request #197: BOOKKEEPER-974: Add an official bookkeeper docker image

2017-07-14 Thread git 
caiok commented on a change in pull request #197: BOOKKEEPER-974: Add an 
official bookkeeper docker image
URL: https://github.com/apache/bookkeeper/pull/197#discussion_r127402908
 
 

 ##
 File path: docker/4.4.0-alpine/run.sh
 ##
 @@ -0,0 +1,68 @@
+#/bin/bash
+
+# -- #
+set -x -e -u
+# -- #
+
+# -- #
+# Allow the container to be started with `--user`
+if [ "$1" = 'bookkeeper' -a "$(id -u)" = '0' ]; then
+chown -R "$BK_USER" "${BK_DIR}" "${BK_JOURNAL_DIR}" "${BK_LEDGER_DIR}" 
"${BK_INDEX_DIR}"
+exec su-exec "$BK_USER" /bin/bash "$0" "$@"
+exit
+fi
+# -- #
+
+# -- #
+# Copy input config files in Bookkeeper configuration directory
+cp -vaf /conf/* ${BK_DIR}/conf || true
+chown -R "$BK_USER" ${BK_DIR}/conf
+
+# Bookkeeper setup
+sed -r -i.bak \
+   -e "s|^journalDirectory.*=.*|journalDirectory=${BK_JOURNAL_DIR}|" \
+   -e "s|^ledgerDirectories.*=.*|ledgerDirectories=${BK_LEDGER_DIR}|" \
+   -e "s|^[# ]*indexDirectories.*=.*|indexDirectories=${BK_INDEX_DIR}|" \
+   -e "s|^[# 
]*useHostNameAsBookieID.*=.*false|useHostNameAsBookieID=true|" \
+   ${BK_DIR}/conf/bk_server.conf
+
+if [[ "${ZK_SERVERS}" != "" ]]; then
+   sed -r -i "s|^zkServers.*=.*|zkServers=${ZK_SERVERS}|" 
${BK_DIR}/conf/bk_server.conf
+fi
+if [[ "${BK_PORT}" != "" ]]; then
+   sed -r -i "s|^bookiePort.*=.*|bookiePort=${BK_PORT}|" 
${BK_DIR}/conf/bk_server.conf
+fi
+if [[ "${BK_LEDGERS_PATH}" != "" ]]; then
+   sed -r -i "s|^[# 
]*zkLedgersRootPath.*=.*|zkLedgersRootPath=${BK_LEDGERS_PATH}|" 
${BK_DIR}/conf/bk_server.conf
+fi
+
+diff ${BK_DIR}/conf/bk_server.conf.bak ${BK_DIR}/conf/bk_server.conf || true
+# -- #
+
+# -- #
+# Wait for zookeeper server
+#set +x
+#zk_server1=$(echo ${ZK_SERVERS} | cut -d"," -f1)
+#zk_server1_host=$(echo ${zk_server1} | cut -d":" -f1)
+#zk_server1_port=$(echo ${zk_server1} | cut -d":" -f2)
+
+#echo -en "\nWaiting for Zookeeper (${zk_server1_host}:${zk_server1_port})..."
+#while [[ $(nc -z ${zk_server1_host} ${zk_server1_port}) -ne 0 ]] ; do
+#  echo -n "."
+#  sleep 2
+#done
+#echo " Connected!"
+#set -x
+# -- #
+
+
+# -- #
+# Trying to format bookkeeper dir on zookeeper. If the dir already exists, it 
does nothing.
 
 Review comment:
   I've messed up someway this line before open the PR, the correct one is 
obviously "exec bookkeeper shell metaformat -nonInteractive || true"
   
   @sijie This command should ignore format if it find zk dir structure already 
formatted, right? What kind of issues can it arise? (just my curiosity)
   @jiazhai I will do metaformat only if it's passed a non empty env var 
BK_TRY_METAFORMAT, it should satisfy both requirements.
 

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: Git protected branches

2017-07-14 Thread Enrico Olivelli 
Yes Flavio,
it is a pity that we cannot ever see in readonly mode the actual
configuration.
If you look at the issue, the operator attached a screenshot in order to
let me see the actual configuration

Maybe as PMC you or at least Sijie can ask for more privileges

-- Enrico

2017-07-14 9:10 GMT+02:00 Flavio Junqueira :

> It's kind of odd that we have to go through infra to do this. Such a
> configuration feels like should be under the control of committers or at
> least the PMC.
>
> Thanks for doing it in any case, Enrico.
>
> -Flavio
>
> > On 13 Jul 2017, at 01:55, Jia Zhai  wrote:
> >
> > 
> >
> > On Thu, Jul 13, 2017 at 5:34 AM, Venkateswara Rao Jujjuri <
> jujj...@gmail.com
> >> wrote:
> >
> >> Awesome. !!
> >>
> >> On Tue, Jul 11, 2017 at 6:06 AM, Enrico Olivelli 
> >> wrote:
> >>
> >>> The ticket has been closed.
> >>> The protection is on
> >>>
> >>> -- Enrico
> >>>
> >>> 2017-07-07 11:22 GMT+02:00 Enrico Olivelli :
> >>>
> 
> 
>  2017-07-07 1:51 GMT+02:00 Sijie Guo :
> 
> > +1 for disable force push to master
> >
> 
> 
>  this is the issue:
>  https://issues.apache.org/jira/browse/INFRA-14535
> 
>  I will send updates
> 
>  - Enrico
> 
> 
> >
> > On Thu, Jul 6, 2017 at 1:16 PM, Enrico Olivelli  >
> > wrote:
> >
> >> Other thoughts?
> >> If there is no objection I would like to enable the force push
> >> block.
> > Next
> >> week
> >>
> >> Enrico
> >>
> >> Il mar 4 lug 2017, 10:28 Enrico Olivelli  ha
> > scritto:
> >>
> >>> 2017-07-04 5:51 GMT+02:00 Jia Zhai :
>  Agree to have them protected.
>  Currently, seems these branches are not protected, at least
> >> master
> >> branch
>  is not. The merge button is still available even "Jenkins: Maven
> > clean
>  install — Build finished." in some of PRs
>  
>  We may first need to make the test stable.
> >>>
> >>> I think that is it enough to only protect against "force push",
> >>> which
> >>> makes impossible for anyone to 'rewrite' the "public history" of
> >> the
> >>> project
> >>> Other checks are performed by QA test and by the committer who
> >> takes
> >>> responsibility for pushing the patch in the repo
> >>>
> >>>
> >>> -- Enrico
> >>>
> >>>
> 
>  On Tue, Jul 4, 2017 at 5:10 AM, Enrico Olivelli <
> > eolive...@gmail.com>
> >>> wrote:
> 
> > Hi,
> > I don't know current configuration but I would like to ensure
> >>> that
> > our
> > release branches, that is master and branch-xx are 'protected'.
> > I mean at least that 'force push' must be forbidden to
> >> everyone.
> >
> > In github it is simple to enable such protection, see
> > https://help.github.com/articles/about-protected-branches/
> >
> > I don't want to try btyy performing a force push. Maybe we can
> >>> ask
> >> infra
> > about current configuration
> >
> > Thoughts?
> >
> > Enrico
> > --
> >
> >
> > -- Enrico Olivelli
> >
> >>>
> >> --
> >>
> >>
> >> -- Enrico Olivelli
> >>
> >
> 
> 
> >>>
> >>
> >>
> >>
> >> --
> >> Jvrao
> >> ---
> >> First they ignore you, then they laugh at you, then they fight you, then
> >> you win. - Mahatma Gandhi
> >>
>
>

Re: Git protected branches

2017-07-14 Thread Flavio Junqueira 
It's kind of odd that we have to go through infra to do this. Such a 
configuration feels like should be under the control of committers or at least 
the PMC.

Thanks for doing it in any case, Enrico.

-Flavio

> On 13 Jul 2017, at 01:55, Jia Zhai  wrote:
> 
> 
> 
> On Thu, Jul 13, 2017 at 5:34 AM, Venkateswara Rao Jujjuri > wrote:
> 
>> Awesome. !!
>> 
>> On Tue, Jul 11, 2017 at 6:06 AM, Enrico Olivelli 
>> wrote:
>> 
>>> The ticket has been closed.
>>> The protection is on
>>> 
>>> -- Enrico
>>> 
>>> 2017-07-07 11:22 GMT+02:00 Enrico Olivelli :
>>> 
 
 
 2017-07-07 1:51 GMT+02:00 Sijie Guo :
 
> +1 for disable force push to master
> 
 
 
 this is the issue:
 https://issues.apache.org/jira/browse/INFRA-14535
 
 I will send updates
 
 - Enrico
 
 
> 
> On Thu, Jul 6, 2017 at 1:16 PM, Enrico Olivelli 
> wrote:
> 
>> Other thoughts?
>> If there is no objection I would like to enable the force push
>> block.
> Next
>> week
>> 
>> Enrico
>> 
>> Il mar 4 lug 2017, 10:28 Enrico Olivelli  ha
> scritto:
>> 
>>> 2017-07-04 5:51 GMT+02:00 Jia Zhai :
 Agree to have them protected.
 Currently, seems these branches are not protected, at least
>> master
>> branch
 is not. The merge button is still available even "Jenkins: Maven
> clean
 install — Build finished." in some of PRs
 
 We may first need to make the test stable.
>>> 
>>> I think that is it enough to only protect against "force push",
>>> which
>>> makes impossible for anyone to 'rewrite' the "public history" of
>> the
>>> project
>>> Other checks are performed by QA test and by the committer who
>> takes
>>> responsibility for pushing the patch in the repo
>>> 
>>> 
>>> -- Enrico
>>> 
>>> 
 
 On Tue, Jul 4, 2017 at 5:10 AM, Enrico Olivelli <
> eolive...@gmail.com>
>>> wrote:
 
> Hi,
> I don't know current configuration but I would like to ensure
>>> that
> our
> release branches, that is master and branch-xx are 'protected'.
> I mean at least that 'force push' must be forbidden to
>> everyone.
> 
> In github it is simple to enable such protection, see
> https://help.github.com/articles/about-protected-branches/
> 
> I don't want to try btyy performing a force push. Maybe we can
>>> ask
>> infra
> about current configuration
> 
> Thoughts?
> 
> Enrico
> --
> 
> 
> -- Enrico Olivelli
> 
>>> 
>> --
>> 
>> 
>> -- Enrico Olivelli
>> 
> 
 
 
>>> 
>> 
>> 
>> 
>> --
>> Jvrao
>> ---
>> First they ignore you, then they laugh at you, then they fight you, then
>> you win. - Mahatma Gandhi
>>