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

[v2]Added Auto Index Rollover #6500

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

Manik2708
Copy link
Contributor

Which problem is this PR solving?

Fixes a part of: #6283

Description of the changes

  • Addition of ES-Rollover (Cron Job) in jaeger binary.

How was this change tested?

  • Unit and e2e tests

Checklist

Signed-off-by: Manik2708 <[email protected]>
Signed-off-by: Manik2708 <[email protected]>
Signed-off-by: Manik2708 <[email protected]>
Signed-off-by: Manik2708 <[email protected]>
Signed-off-by: Manik2708 <[email protected]>
@Manik2708 Manik2708 marked this pull request as draft January 7, 2025 18:58
Signed-off-by: Manik2708 <[email protected]>
Signed-off-by: Manik2708 <[email protected]>
Copy link

codecov bot commented Jan 7, 2025

Codecov Report

Attention: Patch coverage is 47.44186% with 113 lines in your changes missing coverage. Please review.

Project coverage is 95.76%. Comparing base (08503ca) to head (97c4020).

Files with missing lines Patch % Lines
plugin/storage/es/factory.go 1.80% 107 Missing and 2 partials ⚠️
pkg/es/config/config.go 42.85% 3 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6500      +/-   ##
==========================================
- Coverage   96.27%   95.76%   -0.52%     
==========================================
  Files         372      373       +1     
  Lines       21282    21477     +195     
==========================================
+ Hits        20490    20567      +77     
- Misses        605      719     +114     
- Partials      187      191       +4     
Flag Coverage Δ
badger_v1 9.89% <0.00%> (-0.78%) ⬇️
badger_v2 2.57% <0.00%> (-0.21%) ⬇️
cassandra-4.x-v1-manual 15.36% <0.00%> (-1.21%) ⬇️
cassandra-4.x-v2-auto 2.51% <0.00%> (-0.20%) ⬇️
cassandra-4.x-v2-manual 2.51% <0.00%> (-0.20%) ⬇️
cassandra-5.x-v1-manual 15.36% <0.00%> (-1.21%) ⬇️
cassandra-5.x-v2-auto 2.51% <0.00%> (-0.20%) ⬇️
cassandra-5.x-v2-manual 2.51% <0.00%> (-0.20%) ⬇️
elasticsearch-6.x-v1 19.52% <42.32%> (-0.71%) ⬇️
elasticsearch-7.x-v1 19.60% <42.32%> (-0.71%) ⬇️
elasticsearch-8.x-v1 19.74% <42.32%> (-0.73%) ⬇️
elasticsearch-8.x-v2 2.57% <0.00%> (-0.20%) ⬇️
grpc_v1 11.42% <0.00%> (-0.90%) ⬇️
grpc_v2 8.43% <0.00%> (-0.67%) ⬇️
kafka-3.x-v1 9.60% <0.00%> (-0.76%) ⬇️
kafka-3.x-v2 2.57% <0.00%> (-0.21%) ⬇️
memory_v2 2.57% <0.00%> (-0.20%) ⬇️
opensearch-1.x-v1 19.64% <42.32%> (-0.71%) ⬇️
opensearch-2.x-v1 19.64% <42.32%> (-0.73%) ⬇️
opensearch-2.x-v2 2.57% <0.00%> (-0.21%) ⬇️
tailsampling-processor 0.47% <0.00%> (-0.04%) ⬇️
unittests 94.57% <38.60%> (-0.59%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Manik2708 Manik2708 marked this pull request as ready for review January 7, 2025 19:29
@Manik2708
Copy link
Contributor Author

  1. I am not able to write unit tests for factory as it is a cron job which is neither returning any error (it is logging the errors), so need help in the approach to write these tests.
  2. There is an e2e test in ESRollover which is skipped in v2 as jaeger can't exit unlike es-rollover binary. But if this case happens in real, means use_ilm=true in jaeger but there is no policy created, then everytime rollover will fail and log that error, is that fine?
  3. I have not gone with election\follower algorithm as I was able to test the idempotency of rollover. In the e2e tests of jaeger v2 rollover, a frequency of 1s is kept but in test, it doesn't runs only once but multiple times still it passes all the tests.
  4. I am bit confused in the index prefix used in es-rollover and that used in jaeger v2. Are they same? This PR is along this assumption that they are same.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant