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.

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

[bookie-server-config] Avoid considering empty journalDirectories #2245

Merged
merged 1 commit into from Mar 31, 2020
Merged

[bookie-server-config] Avoid considering empty journalDirectories #2245

merged 1 commit into from Mar 31, 2020

Conversation

rdhabalia
Copy link
Contributor

As discussed at : apache/pulsar#6042

Issue

We have config management tool which manages bookie-server configuration and sets empty value of journalDirectories if bookie-server is still wants to use single journal dir: journalDirectory. In this case, bookie-server doesn't consider journalDirectory value and keeps empty list of journalDirectories which stats bookie with invalid configuration.

Expected behavior

Bookie-server should parse empty configuration journalDirectories= properly and avoid picking up empty value of journalDirectories and in that case, bookie-server should fallback to journalDirectory config.

@rdhabalia
Copy link
Contributor Author

ping

@eolivelli
Copy link
Contributor

Why github actions did not trigger?
Btw we can merge this patch

@eolivelli
Copy link
Contributor

trying to close/repone in order to trigger CI

@eolivelli eolivelli closed this Jan 27, 2020
@eolivelli eolivelli reopened this Jan 27, 2020
@rdhabalia
Copy link
Contributor Author

run pr validation

@rdhabalia rdhabalia closed this Jan 27, 2020
@rdhabalia rdhabalia reopened this Jan 27, 2020
@rdhabalia
Copy link
Contributor Author

PR Validation build is failing with below exception:

[ERROR] src/main/java/org/apache/bookkeeper/stream/storage/impl/cluster/ZkClusterInitializer.java:[35] (imports) ImportOrder: Import org.apache.bookkeeper.stream.storage.StorageConstants.getSegmentsRootPath appears after other imports that it should precede
[ERROR] Failed to execute goal org.apache.maven.plugins:maven-checkstyle-plugin:3.0.0:check (default-cli) on project stream-storage-service-impl: You have 1 Checkstyle violation. -> [Help 1]

There is no change for ZkClusterInitializer in the PR. So we know if it's a known issue?

@rdhabalia rdhabalia closed this Jan 29, 2020
@rdhabalia rdhabalia reopened this Jan 29, 2020
@rdhabalia
Copy link
Contributor Author

rdhabalia commented Jan 29, 2020 •
edited

fixed in #2258

@rdhabalia
Copy link
Contributor Author

can we please retrigger the failed jobs.

@sijie
Copy link
Member

sijie commented Jan 31, 2020

@rdhabalia

[ERROR] src/main/java/org/apache/bookkeeper/stream/storage/impl/cluster/ZkClusterInitializer.java:[35] (imports) ImportOrder: Import org.apache.bookkeeper.stream.storage.StorageConstants.getSegmentsRootPath appears after other imports that it should precede
[ERROR] Failed to execute goal org.apache.maven.plugins:maven-checkstyle-plugin:3.0.0:check (default-cli) on project stream-storage-service-impl: You have 1 Checkstyle violation. -> [Help 1]
[ERROR]

Are you able to check the results of github actions?
The PR validation failure doesn't seem to be not a flaky failure.

@eolivelli
Copy link
Contributor

@sijie
I have merged #2258 from @rdhabalia
now the master branch is in good same.
The error was not related to this patch

@rdhabalia
Copy link
Contributor Author

I can't trigger integration test failure with github action because I am not committer. So, is there any other way I can trigger this specific job? or can someone trigger it?

@eolivelli
Copy link
Contributor

@rdhabalia can you please tell about this problem on dev@ list ?
This way we can discuss with the community about how to find a solution

Meanwhile I will take care of kicking CI.

Are integration tests passing on your machine?

@sijie
Copy link
Member

sijie commented Jan 31, 2020

@eolivelli @rdhabalia Github actions doesn't allow people without write permissions to trigger the actions. Even ASF has started disabling trigger phrases for Jenkins as well. I understand it is inconvenient for contributors. we can look into solutions to address them.

However, I think we have replied on trigger phrases too much in the past. These trigger phrases hide the problems in tests. We should start looking into how to make these tests reliable.

@eolivelli
Copy link
Contributor

@rdhabalia I have retriggered the integration tests suite.
I hope we can merge this patch soon

@rdhabalia
Copy link
Contributor Author

thanks @eolivelli , hopefully it passes once we retrigger the job again.. please let me know if you want me to rebase it with latest master changes.

@rdhabalia
Copy link
Contributor Author

can we retrigger this job..

@eolivelli
Copy link
Contributor

Merging as soon as tests pass

@eolivelli
Copy link
Contributor

It failed again

@eolivelli eolivelli closed this Mar 13, 2020
@eolivelli eolivelli reopened this Mar 13, 2020
@eolivelli
Copy link
Contributor

@rdhabalia can you please take a look ?
Try to merge with current master.
Then I will be happy to merge.

@eolivelli
Copy link
Contributor

All tests passed.
I will merge as soon as possible.

thank you @rdhabalia

@rdhabalia rdhabalia self-assigned this Mar 31, 2020
@rdhabalia rdhabalia added this to the 4.11.0 milestone Mar 31, 2020
@rdhabalia rdhabalia merged commit e7430ce into apache:master Mar 31, 2020
@rdhabalia
Copy link
Contributor Author

@eolivelli
I just merged with ./dev/bk-merge-pr.py, can you please let me know if anything I missed or should take care while merging PR?

@rdhabalia rdhabalia deleted the journal_dir branch March 31, 2020 19:13
@eolivelli
Copy link
Contributor

You did it well.
You assigned correct labels.

Usually it is better to not self merge your own issues and let another fellow commiter to merge your patches. But there is no such strict rule it it only about community working.

The rules are: each patch must have two supporting committers. If the contributor is a committer you just only need another approval.

For big changes or changes to the public API we use to issue a BP (Bookkeeper Proposal) and discuss it before discussing the implementation.

It is good to have you on board!

@rdhabalia
Copy link
Contributor Author

I see..thank you for sharing the info..

Ghatage pushed a commit to Ghatage/bookkeeper that referenced this pull request Jun 19, 2020
@rdhabalia @Ghatage
[bookie-server-config] Avoid considering empty journalDirectories
b85eddf 
As discussed at : apache/pulsar#6042

### Issue
We have config management tool which manages bookie-server configuration and sets empty value of `journalDirectories` if bookie-server is still wants to use single journal dir: `journalDirectory`. In this case, bookie-server doesn't consider `journalDirectory` value and keeps empty list of `journalDirectories` which stats bookie with invalid configuration.

### Expected behavior
Bookie-server should parse empty configuration `journalDirectories=` properly and avoid picking up empty value of `journalDirectories` and in that case, bookie-server should fallback to `journalDirectory` config.

Reviewers: Enrico Olivelli <eolivelli@gmail.com>, Sijie Guo <None>

This closes apache#2245 from rdhabalia/journal_dir
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sijie sijie sijie approved these changes

@eolivelli eolivelli eolivelli approved these changes

Assignees

@rdhabalia rdhabalia

Labels
area/bookie area/config release/4.11.0 type/improvement
Projects
None yet
Milestone
4.11.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@rdhabalia @eolivelli @sijie