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

Increase test timeout for TestKafkaStorage #2873

Merged
merged 3 commits into from
Mar 11, 2021
Merged

Increase test timeout for TestKafkaStorage #2873

merged 3 commits into from
Mar 11, 2021

Conversation

LostLaser
Copy link
Contributor

Signed-off-by: Jacob Goldsworthy jtgolds6@gmail.com

Which problem is this PR solving?

Short description of the changes

  • Increase timeout of storage integration tests to allow for longer storage settle time

@LostLaser
Increasing test timeout to allow for trace storage settle
f4bde81 
Signed-off-by: Jacob Goldsworthy <jtgolds6@gmail.com>
@LostLaser LostLaser requested a review from a team as a code owner March 10, 2021 05:42
@LostLaser LostLaser requested a review from vprithvi March 10, 2021 05:42
@codecov
Copy link

codecov bot commented Mar 10, 2021 •
edited
Loading

Codecov Report

Merging #2873 (9e81d8e) into master (305ac76) will decrease coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2873      +/-   ##
==========================================
- Coverage   95.97%   95.95%   -0.03%     
==========================================
  Files         223      223              
  Lines        9685     9685              
==========================================
- Hits         9295     9293       -2     
- Misses        322      324       +2     
  Partials       68       68
Impacted Files Coverage Δ
plugin/storage/integration/integration.go 77.90% <100.00%> (ø)
pkg/config/tlscfg/cert_watcher.go 92.20% <0.00%> (-2.60%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 305ac76...9e81d8e. Read the comment docs.

yurishkuro
yurishkuro reviewed Mar 10, 2021
View reviewed changes
Copy link
Member

@yurishkuro yurishkuro left a comment

Choose a reason for hiding this comment

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

how about using the ACK-ed writes?

plugin/storage/integration/integration.go Outdated
@@ -36,7 +36,7 @@ import (
)

const (
iterations = 30
iterations = 40
Copy link
Member

Choose a reason for hiding this comment

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

go to 100

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Acked writes didn't seem to help unfortunately. I attempted to switch the acks to "local" as well as "all" to no avail. I'm guessing the ack never actually comes into effect because there is probably a timeout on the message transfer that is greater than the test time we have set. I ran kafkacat to see if the spans ever made it to the cluster and it seemed like the messages would always make it after some time.

albertteoh
albertteoh previously approved these changes Mar 10, 2021
View reviewed changes
Copy link
Contributor

@albertteoh albertteoh left a comment

Choose a reason for hiding this comment

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

lgtm

@LostLaser
Merge branch 'master' of https://github.com/jaegertracing/jaeger into…
f6a4d15 
… 2760-flaky-kafka-storage-test
@LostLaser
Updating max storage test wait time to 10 seconds
9e81d8e 
Signed-off-by: Jacob Goldsworthy <jtgolds6@gmail.com>
@mergify mergify bot dismissed albertteoh’s stale review March 11, 2021 02:38

Pull request has been modified.

@yurishkuro yurishkuro changed the title Increasing test timeout to allow for trace storage settle Increasing test timeout for TestKafkaStorage Mar 11, 2021
@yurishkuro yurishkuro merged commit 78019b6 into jaegertracing:master Mar 11, 2021
@yurishkuro yurishkuro changed the title Increasing test timeout for TestKafkaStorage Increase test timeout for TestKafkaStorage Mar 11, 2021
@yurishkuro
Copy link
Member

thanks @LostLaser

@yurishkuro yurishkuro mentioned this pull request Mar 11, 2021
Closed
@dependabot dependabot bot mentioned this pull request Mar 12, 2021
Closed
dgrizzanti pushed a commit to dgrizzanti/jaeger that referenced this pull request Mar 12, 2021
@LostLaser @dgrizzanti
Increase test timeout for TestKafkaStorage (jaegertracing#2873)
5105237 
* Increasing test timeout to allow for trace storage settle

Signed-off-by: Jacob Goldsworthy <jtgolds6@gmail.com>

* Updating max storage test wait time to 10 seconds

Signed-off-by: Jacob Goldsworthy <jtgolds6@gmail.com>
Signed-off-by: David Grizzanti <david_grizzanti@comcast.com>
@albertteoh albertteoh mentioned this pull request Mar 12, 2021
Merged
dgrizzanti pushed a commit to dgrizzanti/jaeger that referenced this pull request Mar 13, 2021
@LostLaser @dgrizzanti
Increase test timeout for TestKafkaStorage (jaegertracing#2873)
5878ecb 
* Increasing test timeout to allow for trace storage settle

Signed-off-by: Jacob Goldsworthy <jtgolds6@gmail.com>

* Updating max storage test wait time to 10 seconds

Signed-off-by: Jacob Goldsworthy <jtgolds6@gmail.com>
Signed-off-by: David Grizzanti <david_grizzanti@comcast.com>
dgrizzanti pushed a commit to dgrizzanti/jaeger that referenced this pull request Mar 13, 2021
@LostLaser @dgrizzanti
Increase test timeout for TestKafkaStorage (jaegertracing#2873)
617e19e 
* Increasing test timeout to allow for trace storage settle

Signed-off-by: Jacob Goldsworthy <jtgolds6@gmail.com>

* Updating max storage test wait time to 10 seconds

Signed-off-by: Jacob Goldsworthy <jtgolds6@gmail.com>
Signed-off-by: David Grizzanti <david_grizzanti@comcast.com>
dgrizzanti pushed a commit to dgrizzanti/jaeger that referenced this pull request Mar 13, 2021
@LostLaser @dgrizzanti
Increase test timeout for TestKafkaStorage (jaegertracing#2873)
8d51bc5 
* Increasing test timeout to allow for trace storage settle

Signed-off-by: Jacob Goldsworthy <jtgolds6@gmail.com>

* Updating max storage test wait time to 10 seconds

Signed-off-by: Jacob Goldsworthy <jtgolds6@gmail.com>
Signed-off-by: David Grizzanti <david_grizzanti@comcast.com>
dgrizzanti pushed a commit to dgrizzanti/jaeger that referenced this pull request Mar 13, 2021
@LostLaser @dgrizzanti
Increase test timeout for TestKafkaStorage (jaegertracing#2873)
80a5403 
* Increasing test timeout to allow for trace storage settle

Signed-off-by: Jacob Goldsworthy <jtgolds6@gmail.com>

* Updating max storage test wait time to 10 seconds

Signed-off-by: Jacob Goldsworthy <jtgolds6@gmail.com>
Signed-off-by: David Grizzanti <david_grizzanti@comcast.com>
yurishkuro pushed a commit that referenced this pull request Mar 16, 2021
@dgrizzanti @allen13 @LostLaser
[es] Add remote read clusters option for cross-cluster querying (#2874)
915d3af 
* Add remote read clusters option to elasticsearch to enable cross-cluster querying

Co-authored-by: allen13 <kex.allen13@gmail.com>
Signed-off-by: David Grizzanti <david_grizzanti@comcast.com>

* Increase test timeout for TestKafkaStorage (#2873)

* Increasing test timeout to allow for trace storage settle

Signed-off-by: Jacob Goldsworthy <jtgolds6@gmail.com>

* Updating max storage test wait time to 10 seconds

Signed-off-by: Jacob Goldsworthy <jtgolds6@gmail.com>
Signed-off-by: David Grizzanti <david_grizzanti@comcast.com>

* Rename indexName to indexPrefix; short circuit addRemoteReadClusters when remoteReadClusters is emtpy

Signed-off-by: David Grizzanti <david_grizzanti@comcast.com>

* Add more test cases for remoteReadClusters with Archive: true and UseReadWriteAliases: true

Signed-off-by: David Grizzanti <david_grizzanti@comcast.com>

* Removing unnecessary changes from this PR

Signed-off-by: David Grizzanti <david_grizzanti@comcast.com>

* Increase test timeout for TestKafkaStorage (#2873)

* Increasing test timeout to allow for trace storage settle

Signed-off-by: Jacob Goldsworthy <jtgolds6@gmail.com>

* Updating max storage test wait time to 10 seconds

Signed-off-by: Jacob Goldsworthy <jtgolds6@gmail.com>
Signed-off-by: David Grizzanti <david_grizzanti@comcast.com>

* Update variable name for consistency

Signed-off-by: David Grizzanti <david_grizzanti@comcast.com>

* Fix index prefix name

Signed-off-by: David Grizzanti <david_grizzanti@comcast.com>

* Prevent remoteReadClusters from being []string{""} when no remote clusters are provided

Signed-off-by: David Grizzanti <david_grizzanti@comcast.com>

Co-authored-by: allen13 <kex.allen13@gmail.com>
Co-authored-by: Jacob G <jtgolds6@gmail.com>
This was referenced Mar 16, 2021
Closed
Closed
@jpkrohling jpkrohling added this to the Release 1.23.0 milestone Jun 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yurishkuro yurishkuro yurishkuro approved these changes

@albertteoh albertteoh albertteoh left review comments

@vprithvi vprithvi Awaiting requested review from vprithvi

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
Release 1.23.0
Development

Successfully merging this pull request may close these issues.
4 participants
@LostLaser @yurishkuro @albertteoh @jpkrohling