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

[WIP]Removed ES-Rollover binary and integrated with Jaeger Binary #6456

Closed
wants to merge 10 commits into from

Conversation

Manik2708
Copy link
Contributor

Which problem is this PR solving?

Fixes a part of: #6283

Description of the changes

  • This is the first step to remove Es Rollover as an external tool and add it as a process in jaeger.
  • This change removes the binary of rollover and integrate it with the cmd/jaeger binary.

How was this change tested?

  • unit and integration tests

Checklist

@Manik2708 Manik2708 requested a review from a team as a code owner January 1, 2025 17:33
@Manik2708 Manik2708 requested a review from albertteoh January 1, 2025 17:33
Copy link

codecov bot commented Jan 1, 2025

Codecov Report

Attention: Patch coverage is 40.00000% with 138 lines in your changes missing coverage. Please review.

Project coverage is 47.88%. Comparing base (e3a883e) to head (f762e15).
Report is 7 commits behind head on main.

Files with missing lines Patch % Lines
plugin/storage/es/factory.go 1.92% 100 Missing and 2 partials ⚠️
storage_v2/v1adapter/archive.go 0.00% 25 Missing ⚠️
pkg/es/config/config.go 0.00% 2 Missing and 1 partial ⚠️
cmd/es-rollover/app/actions.go 0.00% 2 Missing ⚠️
cmd/es-rollover/app/flags.go 0.00% 2 Missing ⚠️
cmd/es-rollover/app/lookback/flags.go 0.00% 2 Missing ⚠️
cmd/es-rollover/app/rollover/flags.go 0.00% 2 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (e3a883e) and HEAD (f762e15). Click for more details.

HEAD has 1 upload less than BASE
Flag BASE (e3a883e) HEAD (f762e15)
unittests 1 0
Additional details and impacted files
@@             Coverage Diff             @@
##             main    #6456       +/-   ##
===========================================
- Coverage   96.26%   47.88%   -48.39%     
===========================================
  Files         370      205      -165     
  Lines       21144    12170     -8974     
===========================================
- Hits        20355     5827    -14528     
- Misses        604     5881     +5277     
- Partials      185      462      +277     
Flag Coverage Δ
badger_v1 9.96% <0.43%> (-0.58%) ⬇️
badger_v2 2.59% <0.00%> (+<0.01%) ⬆️
cassandra-4.x-v1-manual 15.42% <0.43%> (-0.99%) ⬇️
cassandra-4.x-v2-auto 2.53% <0.00%> (+<0.01%) ⬆️
cassandra-4.x-v2-manual 2.53% <0.00%> (-0.02%) ⬇️
cassandra-5.x-v1-manual 15.42% <0.43%> (-0.99%) ⬇️
cassandra-5.x-v2-auto 2.53% <0.00%> (+<0.01%) ⬆️
cassandra-5.x-v2-manual 2.53% <0.00%> (+<0.01%) ⬆️
elasticsearch-6.x-v1 19.62% <40.00%> (-0.52%) ⬇️
elasticsearch-7.x-v1 19.69% <40.00%> (-0.53%) ⬇️
elasticsearch-8.x-v1 19.84% <40.00%> (-0.54%) ⬇️
elasticsearch-8.x-v2 2.58% <0.00%> (-0.10%) ⬇️
grpc_v1 11.50% <0.43%> (-0.69%) ⬇️
grpc_v2 8.49% <0.00%> (-0.49%) ⬇️
kafka-3.x-v1 9.82% <0.43%> (-0.56%) ⬇️
kafka-3.x-v2 2.59% <0.00%> (+<0.01%) ⬆️
memory_v2 2.59% <0.00%> (+<0.01%) ⬆️
opensearch-1.x-v1 19.72% <40.00%> (-0.54%) ⬇️
opensearch-2.x-v1 19.72% <40.00%> (-0.54%) ⬇️
opensearch-2.x-v2 2.58% <0.00%> (-0.01%) ⬇️
tailsampling-processor 0.48% <0.00%> (+0.08%) ⬆️
unittests ?

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.

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.

please undo all build changes. There was no request in the ticket to remove the original es-rollover binary, the request was to make it NOT REQUIRED by incorporating into v2 binary.

@Manik2708
Copy link
Contributor Author

please undo all build changes. There was no request in the ticket to remove the original es-rollover binary, the request was to make it NOT REQUIRED by incorporating into v2 binary.

Okay, sorry for mis-understanding

@Manik2708
Copy link
Contributor Author

Manik2708 commented Jan 1, 2025

@yurishkuro So, binary can be kept as it is but can we move the business logic into pkg and then use it in both jaeger binary and rollover binary?

@yurishkuro
Copy link
Member

why do we need to move business logic? I want to start with smallest changes possible - use the business logic in es.Factory so that it can automatically do the things that standalone binaries were doing. Later we can think about refactoring the location of this business logic.

@Manik2708
Copy link
Contributor Author

The problem is while extending the Config of es, we don't require the whole config files but a small portion of the 3 config files, so where should this shared portion be kept?

@yurishkuro
Copy link
Member

Please be more specific.

@Manik2708
Copy link
Contributor Author

Please be more specific.

  1. There are 4 config files in rollover, all of them have some properties which have some common parameters with config of es (like username and password are already defined in es.Config), so we need not to use those in Conifg. I seperated those parameters which are uniqely required by es.Config, but now the issue is these config options are also used by the rollover binary. So where should we keep these shared configs? In the rollover? In the config or in a new pkg?
  2. While integrating the rollover with v2, we have to use the init, lookback and rollover methods, should they be imported from cmd directory in the factory?

I was confused of managing thiese shared parameters and functions.

@Manik2708 Manik2708 changed the title Removed ES-Rollover binary and integrated with Jaeger Binary [WIP]Removed ES-Rollover binary and integrated with Jaeger Binary Jan 1, 2025
@Manik2708 Manik2708 marked this pull request as draft January 1, 2025 18:32
@yurishkuro
Copy link
Member

You can see PR #6291 (which seems to have stalled) on how we wanted to extend the existing ES Config with the additional parameters required for these tools.

should they be imported from cmd directory in the factory

Yes, it's find to import from there. Like I said, let's implement the capability first, and make sure it's tested in the CI. Then we can safely refactor where the code lives.

Manik2708 and others added 10 commits January 2, 2025 21:12
Signed-off-by: Manik2708 <[email protected]>
…aegertracing#6452)

## Which problem is this PR solving?
- Towards jaegertracing#6337

## Description of the changes
- This PR migrates the v3_api GRPC handler to use the new v2 query
service.

## How was this change tested?
- CI

## Checklist
- [x] I have read
https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md
- [x] I have signed all commits
- [x] I have added unit tests for the new functionality
- [x] I have run lint and test steps successfully
  - for `jaeger`: `make lint test`
  - for `jaeger-ui`: `npm run lint` and `npm run test`

---------

Signed-off-by: Mahad Zaryab <[email protected]>
This reverts commit 3a53124.

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
Copy link
Contributor Author

Closing this, as high number of changes need to be reverted!

@Manik2708 Manik2708 closed this Jan 2, 2025
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.

3 participants