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

Fix flaky tests: ElasticSearchClientTests #12347

Merged
merged 1 commit into from
Oct 14, 2021
Merged

Fix flaky tests: ElasticSearchClientTests #12347

merged 1 commit into from
Oct 14, 2021

Conversation

nicoloboschi
Copy link
Contributor

Motivation

ElasticSearchClientTests#testBulkRetry sometimes fails due to this error

java.lang.AssertionError: expected [2] but found [3]
	at org.testng.Assert.fail(Assert.java:99)
	at org.testng.Assert.failNotEquals(Assert.java:1037)
	at org.testng.Assert.assertEqualsImpl(Assert.java:140)
	at org.testng.Assert.assertEquals(Assert.java:122)
	at org.testng.Assert.assertEquals(Assert.java:797)
	at org.testng.Assert.assertEquals(Assert.java:807)
	at org.apache.pulsar.io.elasticsearch.ElasticSearchClientTests.testBulkRetry(ElasticSearchClientTests.java:207)

Modifications

  • The reason why the test fail is that the background bulk thread does some flush at certain intervals. In this tests we wants to check the explicit flush() method, so I disabled the periodic flusher thread
  • Fixed leaks in ElasticSearchClientTests test class where tests weren't calling ElasticSearchClient#close() method, so some daemon threads never stop until the java process has been done

Verifying this change

This change is a trivial rework / code cleanup without any test coverage.

@nicoloboschi
Copy link
Contributor Author

/pulsarbot run-failure-checks

1 similar comment
@nicoloboschi
Copy link
Contributor Author

/pulsarbot run-failure-checks

@Anonymitaet
Copy link
Member

@nicoloboschi Thanks for your contribution. For this PR, do we need to update docs?

(The PR template contains info about doc, which helps others know more about the changes. Can you provide doc-related info in this and future PR descriptions? Thanks)

@nicoloboschi
Copy link
Contributor Author

@Anonymitaet the documentation does not need to be updated. I skipped doc part since I choose the template option "This change is a trivial rework / code cleanup without any test coverage.", btw, in the next PRs, I will keep the doc-related info

eolivelli
eolivelli approved these changes Oct 14, 2021
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.

Lgtm

lhotari
lhotari approved these changes Oct 14, 2021
View reviewed changes
Copy link
Member

@lhotari lhotari left a comment

Choose a reason for hiding this comment

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

LGTM

@eolivelli eolivelli merged commit 77fb13e into apache:master Oct 14, 2021
@eolivelli
Copy link
Contributor

eolivelli commented Oct 14, 2021 •
edited
Loading

Committed to master branch.
I will cherry pick to branch-2.9

Thank you @nicoloboschi

@Anonymitaet Anonymitaet added the doc-not-needed Your PR changes do not impact docs label Oct 14, 2021
zeo1995 pushed a commit to zeo1995/pulsar that referenced this pull request Oct 14, 2021
Merge branch 'up/master' into website/branch-2.7.2
82208c9 
* up/master: (37 commits)
  re-enabling integration tests for Sinks (apache#12307)
  [PIP 95][Issue 12040][web] Topic lookup with listener header (apache#12072)
  Fix the master CI broken with update dispatch rate block issue (apache#12360)
  Fix message being ignored when the non-persistent topic reader reconnect. (apache#12348)
  Fix log format. (apache#12346)
  [website][upgrade]feat: docs migration - version-2.7.2 Concepts and Architecture (apache#12354)
  [website][upgrade] feat: full docs migration for version 2.8.0 (apache#12359)
  [website][upgrade]feat: dynamic replace version info before build (apache#12337)
  Fix flaky tests: ElasticSearchClientTests (apache#12347)
  Use asyncCloseCursorLedger to replace cursorLedger.asyncClose method in the ManagedCursorImpl.VoidCallback#operationComplete (apache#12113)
  fix-npe-ZkBookieRackAffinityMapping (apache#11947)
  [pulsar-admin] Allow setting --forward-source-message-property to false when updating a pulsar function (apache#12128)
  [website][upgrade]feat: docs migration - Development (apache#12320)
  Update delete inactive topic configuration documentation (apache#12350)
  [PIP 95][Issue 12040][broker] Multiple bind addresses for Pulsar protocol (apache#12056)
  Added Debezium Source for MS SQL Server (apache#12256)
  Fix: flaky oracle tests (apache#12306)
  [C++] Use URL encoded content type for OAuth 2.0 authentication (apache#12341)
  [C++] Handle OAuth 2.0 exceptional cases gracefully (apache#12335)
  feat(cli): add restart command to pulsar-daemon (apache#12279)
  ...

# Conflicts:
#	site2/website-next/docusaurus.config.js
#	site2/website-next/versioned_sidebars/version-2.7.2-sidebars.json
#	site2/website-next/versions.json
bharanic-dev pushed a commit to bharanic-dev/pulsar that referenced this pull request Mar 18, 2022
@nicoloboschi @ciaocloud
Fix flaky tests: ElasticSearchClientTests (apache#12347)
d7cb66c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eolivelli eolivelli eolivelli approved these changes

@lhotari lhotari lhotari approved these changes

Assignees
No one assigned
Labels
doc-not-needed Your PR changes do not impact docs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@nicoloboschi @Anonymitaet @eolivelli @lhotari