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.

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

Increase test timeout for TestKafkaStorage #2873

Merged

Conversation

LostLaser
Copy link
Contributor

Signed-off-by: Jacob Goldsworthy [email protected]

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 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

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.

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?

@@ -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
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

@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

dgrizzanti pushed a commit to dgrizzanti/jaeger that referenced this pull request Mar 12, 2021
* Increasing test timeout to allow for trace storage settle

Signed-off-by: Jacob Goldsworthy <[email protected]>

* Updating max storage test wait time to 10 seconds

Signed-off-by: Jacob Goldsworthy <[email protected]>
Signed-off-by: David Grizzanti <[email protected]>
dgrizzanti pushed a commit to dgrizzanti/jaeger that referenced this pull request Mar 13, 2021
* Increasing test timeout to allow for trace storage settle

Signed-off-by: Jacob Goldsworthy <[email protected]>

* Updating max storage test wait time to 10 seconds

Signed-off-by: Jacob Goldsworthy <[email protected]>
Signed-off-by: David Grizzanti <[email protected]>
dgrizzanti pushed a commit to dgrizzanti/jaeger that referenced this pull request Mar 13, 2021
* Increasing test timeout to allow for trace storage settle

Signed-off-by: Jacob Goldsworthy <[email protected]>

* Updating max storage test wait time to 10 seconds

Signed-off-by: Jacob Goldsworthy <[email protected]>
Signed-off-by: David Grizzanti <[email protected]>
dgrizzanti pushed a commit to dgrizzanti/jaeger that referenced this pull request Mar 13, 2021
* Increasing test timeout to allow for trace storage settle

Signed-off-by: Jacob Goldsworthy <[email protected]>

* Updating max storage test wait time to 10 seconds

Signed-off-by: Jacob Goldsworthy <[email protected]>
Signed-off-by: David Grizzanti <[email protected]>
dgrizzanti pushed a commit to dgrizzanti/jaeger that referenced this pull request Mar 13, 2021
* Increasing test timeout to allow for trace storage settle

Signed-off-by: Jacob Goldsworthy <[email protected]>

* Updating max storage test wait time to 10 seconds

Signed-off-by: Jacob Goldsworthy <[email protected]>
Signed-off-by: David Grizzanti <[email protected]>
yurishkuro pushed a commit that referenced this pull request Mar 16, 2021
* Add remote read clusters option to elasticsearch to enable cross-cluster querying

Co-authored-by: allen13 <[email protected]>
Signed-off-by: David Grizzanti <[email protected]>

* Increase test timeout for TestKafkaStorage (#2873)

* Increasing test timeout to allow for trace storage settle

Signed-off-by: Jacob Goldsworthy <[email protected]>

* Updating max storage test wait time to 10 seconds

Signed-off-by: Jacob Goldsworthy <[email protected]>
Signed-off-by: David Grizzanti <[email protected]>

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

Signed-off-by: David Grizzanti <[email protected]>

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

Signed-off-by: David Grizzanti <[email protected]>

* Removing unnecessary changes from this PR

Signed-off-by: David Grizzanti <[email protected]>

* Increase test timeout for TestKafkaStorage (#2873)

* Increasing test timeout to allow for trace storage settle

Signed-off-by: Jacob Goldsworthy <[email protected]>

* Updating max storage test wait time to 10 seconds

Signed-off-by: Jacob Goldsworthy <[email protected]>
Signed-off-by: David Grizzanti <[email protected]>

* Update variable name for consistency

Signed-off-by: David Grizzanti <[email protected]>

* Fix index prefix name

Signed-off-by: David Grizzanti <[email protected]>

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

Signed-off-by: David Grizzanti <[email protected]>

Co-authored-by: allen13 <[email protected]>
Co-authored-by: Jacob G <[email protected]>
@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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants