Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

check indexBaseDir specified with ledgerBaseDir #3967

Merged
merged 3 commits into from
May 31, 2023

Conversation

wenbingshen
Copy link
Member

@wenbingshen wenbingshen commented May 23, 2023

Motivation

I deployed the pulsar-3.0.0 version, and I observed the log of bookkeeper-4.16.1 as follows, I did not config special indexDirectories, but the printed log told me that indexDirectories was specified, which is wrong.

2023-05-23T19:52:45,192+0800 [main] INFO  org.apache.bookkeeper.bookie.storage.ldb.SingleDirectoryDbLedgerStorage - indexDir is specified, creating single directory db ledger storage on /data0/pulsar/data/ledgers/current
2023-05-23T19:52:45,322+0800 [main] INFO  org.apache.bookkeeper.bookie.storage.ldb.KeyValueStorageRocksDB - Searching for a RocksDB configuration file in /usr/local/pulsar/apache-pulsar-3.0.0/conf/ledger_metadata_rocksdb.conf
2023-05-23T19:52:45,322+0800 [main] INFO  org.apache.bookkeeper.bookie.storage.ldb.KeyValueStorageRocksDB - Found a RocksDB configuration file and using it to initialize the RocksDB
2023-05-23T19:52:45,337+0800 [main] INFO  org.apache.bookkeeper.bookie.storage.ldb.KeyValueStorageRocksDB - Searching for a RocksDB configuration file in /usr/local/pulsar/apache-pulsar-3.0.0/conf/entry_location_rocksdb.conf
2023-05-23T19:52:45,337+0800 [main] INFO  org.apache.bookkeeper.bookie.storage.ldb.KeyValueStorageRocksDB - Found a RocksDB configuration file and using it to initialize the RocksDB

here ledgerDirsManager == indexDirsManager, so ledgerDir is always equals indexDir.

https://github.com/apache/bookkeeper/blob/master/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/storage/ldb/DbLedgerStorage.java#L181

for (int i = 0; i < ledgerDirsManager.getAllLedgerDirs().size(); i++) {
            File ledgerDir = ledgerDirsManager.getAllLedgerDirs().get(i);
            File indexDir = indexDirsManager.getAllLedgerDirs().get(i);
            // Create a ledger dirs manager for the single directory
            File[] lDirs = new File[1];
            // Remove the `/current` suffix which will be appended again by LedgersDirManager
            lDirs[0] = ledgerDir.getParentFile();
            LedgerDirsManager ldm = new LedgerDirsManager(conf, lDirs, ledgerDirsManager.getDiskChecker(),
                    NullStatsLogger.INSTANCE);

            // Create a index dirs manager for the single directory
            File[] iDirs = new File[1];
            // Remove the `/current` suffix which will be appended again by LedgersDirManager
            iDirs[0] = indexDir.getParentFile();
            LedgerDirsManager idm = new LedgerDirsManager(conf, iDirs, indexDirsManager.getDiskChecker(),
                    NullStatsLogger.INSTANCE);

             ......

            ledgerStorageList.add(newSingleDirectoryDbLedgerStorage(conf, ledgerManager, ldm,
                idm, entrylogger,
                statsLogger, perDirectoryWriteCacheSize,
                perDirectoryReadCacheSize,
                readAheadCacheBatchSize, readAheadCacheBatchBytesSize));

              ......
}

Copy link
Member

@horizonzy horizonzy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's hard to judge whether the index is specified or not. The user may use config as followings:

ledgerDirectories=/Users/horizon/Downloads/bk1/bk-data
indexDirectories=/Users/horizon/Downloads/bk1/bk-data

Copy link
Member

@horizonzy horizonzy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's hard to judge whether the index is specified or not. The user may use config as followings:

ledgerDirectories=/Users/horizon/Downloads/bk1/bk-data
indexDirectories=/Users/horizon/Downloads/bk1/bk-data

Copy link
Member

@horizonzy horizonzy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want to differ it.
The best place is
https://github.com/apache/bookkeeper/blob/c924cfeb509c4e65e33a82ae88ad423017edb669/bookkeeper-server/src/main/java/org/apache/bookkeeper/server/EmbeddedServer.java#LL332C12-L332C12

We would better print the log to show the indexDir is not config here.

In the SingleDirectoryDbLedgerStorage, print indexBaseDir directly.

@wenbingshen
Copy link
Member Author

ledgerDirectories=/Users/horizon/Downloads/bk1/bk-data
indexDirectories=/Users/horizon/Downloads/bk1/bk-data

@horizonzy My first thought was to ignore this particular case, there is indeed some laziness here. Hahaha :)

I updated the log output. PTAL.

Copy link
Member

@horizonzy horizonzy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@wenbingshen
Copy link
Member Author

rerun failure checks

@wenbingshen
Copy link
Member Author

PR Validation stuck for a long time, so I reopened the PR.
image

@wenbingshen wenbingshen reopened this May 26, 2023
Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

@wenbingshen
Copy link
Member Author

rerun failure checks

2 similar comments
@wenbingshen
Copy link
Member Author

rerun failure checks

@wenbingshen
Copy link
Member Author

rerun failure checks

@hangc0276 hangc0276 added this to the 4.17.0 milestone May 30, 2023
@hangc0276 hangc0276 merged commit e93a7f1 into apache:master May 31, 2023
@wenbingshen wenbingshen deleted the wenbing/checkIDM branch May 31, 2023 05:12
zymap pushed a commit that referenced this pull request Jun 19, 2023
### Motivation

I deployed the pulsar-3.0.0 version, and I observed the log of bookkeeper-4.16.1 as follows, I did not config special indexDirectories, but the printed log told me that indexDirectories was specified, which is wrong.

```
2023-05-23T19:52:45,192+0800 [main] INFO  org.apache.bookkeeper.bookie.storage.ldb.SingleDirectoryDbLedgerStorage - indexDir is specified, creating single directory db ledger storage on /data0/pulsar/data/ledgers/current
2023-05-23T19:52:45,322+0800 [main] INFO  org.apache.bookkeeper.bookie.storage.ldb.KeyValueStorageRocksDB - Searching for a RocksDB configuration file in /usr/local/pulsar/apache-pulsar-3.0.0/conf/ledger_metadata_rocksdb.conf
2023-05-23T19:52:45,322+0800 [main] INFO  org.apache.bookkeeper.bookie.storage.ldb.KeyValueStorageRocksDB - Found a RocksDB configuration file and using it to initialize the RocksDB
2023-05-23T19:52:45,337+0800 [main] INFO  org.apache.bookkeeper.bookie.storage.ldb.KeyValueStorageRocksDB - Searching for a RocksDB configuration file in /usr/local/pulsar/apache-pulsar-3.0.0/conf/entry_location_rocksdb.conf
2023-05-23T19:52:45,337+0800 [main] INFO  org.apache.bookkeeper.bookie.storage.ldb.KeyValueStorageRocksDB - Found a RocksDB configuration file and using it to initialize the RocksDB
```

here ledgerDirsManager == indexDirsManager, so ledgerDir is always equals indexDir.

https://github.com/apache/bookkeeper/blob/master/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/storage/ldb/DbLedgerStorage.java#L181
```java
for (int i = 0; i < ledgerDirsManager.getAllLedgerDirs().size(); i++) {
            File ledgerDir = ledgerDirsManager.getAllLedgerDirs().get(i);
            File indexDir = indexDirsManager.getAllLedgerDirs().get(i);
            // Create a ledger dirs manager for the single directory
            File[] lDirs = new File[1];
            // Remove the `/current` suffix which will be appended again by LedgersDirManager
            lDirs[0] = ledgerDir.getParentFile();
            LedgerDirsManager ldm = new LedgerDirsManager(conf, lDirs, ledgerDirsManager.getDiskChecker(),
                    NullStatsLogger.INSTANCE);

            // Create a index dirs manager for the single directory
            File[] iDirs = new File[1];
            // Remove the `/current` suffix which will be appended again by LedgersDirManager
            iDirs[0] = indexDir.getParentFile();
            LedgerDirsManager idm = new LedgerDirsManager(conf, iDirs, indexDirsManager.getDiskChecker(),
                    NullStatsLogger.INSTANCE);

             ......

            ledgerStorageList.add(newSingleDirectoryDbLedgerStorage(conf, ledgerManager, ldm,
                idm, entrylogger,
                statsLogger, perDirectoryWriteCacheSize,
                perDirectoryReadCacheSize,
                readAheadCacheBatchSize, readAheadCacheBatchBytesSize));

              ......
}
```

(cherry picked from commit e93a7f1)
Ghatage pushed a commit to sijie/bookkeeper that referenced this pull request Jul 12, 2024
### Motivation

I deployed the pulsar-3.0.0 version, and I observed the log of bookkeeper-4.16.1 as follows, I did not config special indexDirectories, but the printed log told me that indexDirectories was specified, which is wrong.

```
2023-05-23T19:52:45,192+0800 [main] INFO  org.apache.bookkeeper.bookie.storage.ldb.SingleDirectoryDbLedgerStorage - indexDir is specified, creating single directory db ledger storage on /data0/pulsar/data/ledgers/current
2023-05-23T19:52:45,322+0800 [main] INFO  org.apache.bookkeeper.bookie.storage.ldb.KeyValueStorageRocksDB - Searching for a RocksDB configuration file in /usr/local/pulsar/apache-pulsar-3.0.0/conf/ledger_metadata_rocksdb.conf
2023-05-23T19:52:45,322+0800 [main] INFO  org.apache.bookkeeper.bookie.storage.ldb.KeyValueStorageRocksDB - Found a RocksDB configuration file and using it to initialize the RocksDB
2023-05-23T19:52:45,337+0800 [main] INFO  org.apache.bookkeeper.bookie.storage.ldb.KeyValueStorageRocksDB - Searching for a RocksDB configuration file in /usr/local/pulsar/apache-pulsar-3.0.0/conf/entry_location_rocksdb.conf
2023-05-23T19:52:45,337+0800 [main] INFO  org.apache.bookkeeper.bookie.storage.ldb.KeyValueStorageRocksDB - Found a RocksDB configuration file and using it to initialize the RocksDB
```

here ledgerDirsManager == indexDirsManager, so ledgerDir is always equals indexDir.

https://github.com/apache/bookkeeper/blob/master/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/storage/ldb/DbLedgerStorage.java#L181
```java
for (int i = 0; i < ledgerDirsManager.getAllLedgerDirs().size(); i++) {
            File ledgerDir = ledgerDirsManager.getAllLedgerDirs().get(i);
            File indexDir = indexDirsManager.getAllLedgerDirs().get(i);
            // Create a ledger dirs manager for the single directory
            File[] lDirs = new File[1];
            // Remove the `/current` suffix which will be appended again by LedgersDirManager
            lDirs[0] = ledgerDir.getParentFile();
            LedgerDirsManager ldm = new LedgerDirsManager(conf, lDirs, ledgerDirsManager.getDiskChecker(),
                    NullStatsLogger.INSTANCE);

            // Create a index dirs manager for the single directory
            File[] iDirs = new File[1];
            // Remove the `/current` suffix which will be appended again by LedgersDirManager
            iDirs[0] = indexDir.getParentFile();
            LedgerDirsManager idm = new LedgerDirsManager(conf, iDirs, indexDirsManager.getDiskChecker(),
                    NullStatsLogger.INSTANCE);

             ......

            ledgerStorageList.add(newSingleDirectoryDbLedgerStorage(conf, ledgerManager, ldm,
                idm, entrylogger,
                statsLogger, perDirectoryWriteCacheSize,
                perDirectoryReadCacheSize,
                readAheadCacheBatchSize, readAheadCacheBatchBytesSize));

              ......
}
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants