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

[tests] Fix integration test TestCompatUpgradeYahooCustom.testUpgradeYahooCustom #1772

Merged
merged 1 commit into from Oct 30, 2018
Merged

[tests] Fix integration test TestCompatUpgradeYahooCustom.testUpgradeYahooCustom #1772

merged 1 commit into from Oct 30, 2018

Conversation

sijie
Copy link
Member

@sijie sijie commented Oct 30, 2018

Descriptions of the changes in this PR:

Motivation

#1749 fixes bash script issue but it introduced set -e which
will fail bin/bookkeeper if grepping configuration file failed.

Changes

set -e is useful to fail fast if anything fails in the bash script. This change
is to mask exit code of grep operation, since the bash script already handle
the grep result.

Tests

Manually verified the change works. And the existing TestCompatUpgradeYahooCustom.testUpgradeYahooCustom
integration tests would also verify if the change fixes the problem or not.

In order to uphold a high standard for quality for code contributions, Apache BookKeeper runs various precommit
checks for pull requests. A pull request can only be merged when it passes precommit checks. However running all
the precommit checks can take a long time, some trivial changes don't need to run all the precommit checks. You
can check following list to skip the tests that don't need to run for your pull request. Leave them unchecked if
you are not sure, committers will help you:

  • [skip bookkeeper-server bookie tests]: skip testing org.apache.bookkeeper.bookie in bookkeeper-server module.
  • [skip bookkeeper-server client tests]: skip testing org.apache.bookkeeper.client in bookkeeper-server module.
  • [skip bookkeeper-server replication tests]: skip testing org.apache.bookkeeper.replication in bookkeeper-server module.
  • [skip bookkeeper-server tls tests]: skip testing org.apache.bookkeeper.tls in bookkeeper-server module.
  • [skip bookkeeper-server remaining tests]: skip testing all other tests in bookkeeper-server module.
  • [skip integration tests]: skip docker based integration tests. if you make java code changes, you shouldn't skip integration tests.
  • [skip build java8]: skip build on java8. ONLY skip this when ONLY changing files under documentation under site.
  • [skip build java9]: skip build on java9. ONLY skip this when ONLY changing files under documentation under site.

Be sure to do all of the following to help us incorporate your contribution
quickly and easily:

If this PR is a BookKeeper Proposal (BP):

  • Make sure the PR title is formatted like:
    <BP-#>: Description of bookkeeper proposal
    e.g. BP-1: 64 bits ledger is support
  • Attach the master issue link in the description of this PR.
  • Attach the google doc link if the BP is written in Google Doc.

Otherwise:

  • Make sure the PR title is formatted like:
    <Issue #>: Description of pull request
    e.g. Issue 123: Description ...
  • Make sure tests pass via mvn clean apache-rat:check install spotbugs:check.
  • Replace <Issue #> in the title with the actual Issue number.

@sijie
[tests] Fix integration test TestCompatUpgradeYahooCustom.testUpgrade…
7b200f7 
…YahooCustom

*Motivation*

apache#1749 fixes bash script issue but it introduced `set -e` which
will fail `bin/bookkeeper` if grepping configuration file failed.

*Changes*

`set -e` is useful to fail fast if anything fails in the bash script. This change
is to mask exit code of `grep` operation, since the bash script already handle
the grep result.

*Tests*

Manually verified the change works. And the existing TestCompatUpgradeYahooCustom.testUpgradeYahooCustom
integration tests would also verify if the change fixes the problem or not.
@sijie sijie added this to the 4.9.0 milestone Oct 30, 2018
@sijie sijie self-assigned this Oct 30, 2018
@sijie sijie requested a review from ivankelly October 30, 2018 02:05
@sijie
Copy link
Member Author

sijie commented Oct 30, 2018

@ivankelly can you review this?

@eolivelli
Copy link
Contributor

rebuild java9

eolivelli
eolivelli approved these changes Oct 30, 2018
View reviewed changes
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.

Integration tests passed

@eolivelli
Copy link
Contributor

So we still have to continue supporting this upgrade path (from Yahoo I mean)?

It has been some time ago that Yahoo backported all of the changes for Pulsar.

@sijie
Copy link
Member Author

sijie commented Oct 30, 2018

So we still have to continue supporting this upgrade path (from Yahoo I mean)?

This is for BC testing. Until we have EOL, I don't see a reason to remove this.

@sijie
Copy link
Member Author

sijie commented Oct 30, 2018

rebuild java9

@ivankelly
Copy link
Contributor

@eolivelli

So we still have to continue supporting this upgrade path (from Yahoo I mean)?

The test is only catching a more general issue. If someone brings a bk_server.conf from an older version, it should still work.

@sijie sijie merged commit 676dc82 into apache:master Oct 30, 2018
@sijie sijie deleted the fix_common_scripts branch October 30, 2018 18:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ivankelly ivankelly ivankelly approved these changes

@eolivelli eolivelli eolivelli approved these changes

Assignees

@sijie sijie

Labels
area/bookie area/tools release/4.9.0 type/bug
Projects
None yet
Milestone
4.9.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@sijie @eolivelli @ivankelly