Skip to content

Fix RocksDB configuration path handling on Windows #4407

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.

Sign up for GitHub

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

Jump to bottom
Merged
hezhangjian merged 1 commit into from
May 31, 2024
Merged

Fix RocksDB configuration path handling on Windows #4407

hezhangjian merged 1 commit into from
May 31, 2024

Conversation

@hezhangjian
Copy link
Member

@hezhangjian hezhangjian commented May 30, 2024

Motivation

This PR addresses the issue where RocksDB configurations fail to correctly resolve paths on Windows systems in the ServerConfiguration class. See windows daily build failed in https://github.com/apache/bookkeeper/actions/runs/9294883674

Note: Currently, windows is limited to some development jobs, there are still problems to be solved

Changes

  • Updated ServerConfiguration.java to utilize java.nio.file.Paths for path normalization and resolution.
  • Refactored the getDefaultRocksDBConf, getEntryLocationRocksdbConf, and getLedgerMetadataRocksdbConf methods to use a new method getFilePath.

Fix RocksDB configuration path handling on Windows
e67923c 
Signed-off-by: ZhangJian He <shoothzj@gmail.com>
@hezhangjian hezhangjian added this to the 4.18.0 milestone May 30, 2024
@hezhangjian hezhangjian self-assigned this May 30, 2024
@dlg99
Copy link
Contributor

dlg99 commented May 30, 2024

I see

Error:  org.apache.bookkeeper.replication.BookieAutoRecoveryTest.testOpenLedgers -- Time elapsed: 120.0 s <<< ERROR!
org.junit.runners.model.TestTimedOutException: test timed out after 120 seconds

I kicked CI retry but this test was stable IIRC.

@hezhangjian
Copy link
Member Author

hezhangjian commented May 31, 2024

@dlg99 I saw you open flaky test issues, I will take a look. I think we can approve this first. It solves error like this:

Error: Run 2: TestReadLastConfirmedAndEntry>BookKeeperClusterTestCase.setUp:169->BookKeeperClusterTestCase.setUp:194->BookKeeperClusterTestCase.startBKCluster:286->BookKeeperClusterTestCase.startNewBookie:658->BookKeeperClusterTestCase.startNewBookieAndReturnAddress:665->BookKeeperClusterTestCase.startAndAddBookie:676->BookKeeperClusterTestCase.startBookie:696 » InvalidPath Illegal char <:> at index 2: /D:/a/bookkeeper/bookkeeper/bookkeeper-server/target/test-classes/conf/ledger_metadata_rocksdb.conf

@dlg99
Copy link
Contributor

dlg99 commented May 31, 2024

I first noticed this test failing on this PR and later checked the others. LGTM, not broken by this change

@hezhangjian hezhangjian merged commit d7b2df4 into apache:master May 31, 2024
@hezhangjian hezhangjian deleted the fix-widnwos-rocksdb branch May 31, 2024 03:45
Ghatage pushed a commit to sijie/bookkeeper that referenced this pull request Jul 12, 2024
Fix RocksDB configuration path handling on Windows (apache#4407)
582f271 
### Motivation

This PR addresses the issue where RocksDB configurations fail to correctly resolve paths on Windows systems in the `ServerConfiguration` class.

### Changes

- Updated `ServerConfiguration.java` to utilize `java.nio.file.Paths` for path normalization and resolution.
- Refactored the `getDefaultRocksDBConf`, `getEntryLocationRocksdbConf`, and `getLedgerMetadataRocksdbConf` methods to use a new method `getFilePath`.

Signed-off-by: ZhangJian He <shoothzj@gmail.com>
This was referenced Mar 5, 2025
Merged
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dlg99 dlg99 dlg99 approved these changes

@merlimat merlimat Awaiting requested review from merlimat

@hangc0276 hangc0276 Awaiting requested review from hangc0276

@eolivelli eolivelli Awaiting requested review from eolivelli

@nicoloboschi nicoloboschi Awaiting requested review from nicoloboschi

Assignees

@hezhangjian hezhangjian

Labels

area/bookie

Projects

None yet

Milestone

4.18.0

Development

Successfully merging this pull request may close these issues.

2 participants

@hezhangjian @dlg99