You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@bookkeeper.apache.org by GitBox <gi...@apache.org> on 2020/10/26 15:50:22 UTC
[GitHub] [bookkeeper] nicoloboschi opened a new pull request #2457: BP-42: New Client API - list ledgers
nicoloboschi opened a new pull request #2457:
URL: https://github.com/apache/bookkeeper/pull/2457
Implementation of BP-42
### Motivation
Enhance new client API, added methods for
- listing ledgers
- access to ledger metadata
### Changes
- Added new interfaces in "api" client package for ledgers listing
- Added new method `getLedgerMetadata(long ledgerId)` to BookKeeper interface
- Added `getLedgerId()` to LedgerMetadata type
-- Ledger metadata ser/deser are not changed since ledgerId is not stored inside ZK data node
Updated all tests using `LedgerMetadataBuilder` because ledgerId is now required in order to call `build()` method
Master Issue: #2422
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] eolivelli commented on a change in pull request #2457: BP-42: New Client API - list ledgers
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #2457:
URL: https://github.com/apache/bookkeeper/pull/2457#discussion_r512097588
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerMetadataBuilder.java
##########
@@ -190,10 +197,11 @@ public LedgerMetadataBuilder withCToken(long cToken) {
}
public LedgerMetadata build() {
+ checkArgument(ledgerId > 0, "Ledger id must be set");
Review comment:
ledgerId = 0 is a valid value AFAIK
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/client/api/ListLedgersResultBuilder.java
##########
@@ -0,0 +1,34 @@
+/**
+ *
+ * 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.client.api;
+
+import org.apache.bookkeeper.common.annotation.InterfaceAudience.Public;
+import org.apache.bookkeeper.common.annotation.InterfaceStability.Unstable;
+
+/**
+ * Builder-style interface to list exiting ledgers.
+ *
Review comment:
nit: remove empty line
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeper.java
##########
@@ -1473,6 +1480,119 @@ public DeleteBuilder newDeleteLedgerOp() {
return new LedgerDeleteOp.DeleteBuilderImpl(this);
}
+ private static final class SyncLedgerIterator implements LedgersIterator {
+
+ private final LedgerRangeIterator iterator;
+ private final ListLedgersResultImpl parent;
+ Iterator<Long> currentRange = null;
+
+ public SyncLedgerIterator(LedgerRangeIterator iterator, ListLedgersResultImpl parent) {
+ this.iterator = iterator;
+ this.parent = parent;
+ }
+
+ @Override
+ public boolean hasNext() throws IOException {
+ parent.checkClosed();
+ if (currentRange != null) {
+ if (currentRange.hasNext()) {
+ return true;
+ }
+ } else if (iterator.hasNext()) {
+ return true;
+ }
+ return false;
+ }
+
+ @Override
+ public long next() throws IOException {
+ parent.checkClosed();
+ if (currentRange == null || !currentRange.hasNext()) {
+ currentRange = iterator.next().getLedgers().iterator();
+ }
+ return currentRange.next();
+ }
+ }
+
+ private static final class ListLedgersResultImpl implements ListLedgersResult {
+
+ private final LedgerRangeIterator iterator;
+ private boolean closed = false;
+ private LedgersIterator ledgersIterator;
+
+ public ListLedgersResultImpl(LedgerRangeIterator iterator) {
+ this.iterator = iterator;
+ }
+
+ void checkClosed() {
+ if (closed) {
+ throw new IllegalStateException("ListLedgersResult is closed");
+ }
+ }
+
+ private void initLedgersIterator() {
+ if (ledgersIterator != null) {
+ throw new IllegalStateException("LedgersIterator must be requested once");
+ }
+ ledgersIterator = new SyncLedgerIterator(iterator, this);
+ }
+
+ @Override
+ public LedgersIterator iterator() {
+ checkClosed();
+ initLedgersIterator();
+ return ledgersIterator;
+
Review comment:
nit: remove empty line
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] eolivelli commented on pull request #2457: BP-42: New Client API - list ledgers
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #2457:
URL: https://github.com/apache/bookkeeper/pull/2457#issuecomment-717835852
rerun failure checks
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] nicoloboschi commented on pull request #2457: BP-42: New Client API - list ledgers
Posted by GitBox <gi...@apache.org>.
nicoloboschi commented on pull request #2457:
URL: https://github.com/apache/bookkeeper/pull/2457#issuecomment-717818103
@eolivelli I made some changes as requested
Checks passes except for SlowBookieTest but I think it is flaky, running it locally passes
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] eolivelli commented on pull request #2457: BP-42: New Client API - list ledgers
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #2457:
URL: https://github.com/apache/bookkeeper/pull/2457#issuecomment-717834092
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] eolivelli merged pull request #2457: BP-42: New Client API - list ledgers
Posted by GitBox <gi...@apache.org>.
eolivelli merged pull request #2457:
URL: https://github.com/apache/bookkeeper/pull/2457
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org