[v2]Added Auto Index Rollover #6500
Conversation
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]>
Signed-off-by: Manik Mehta <[email protected]>
Signed-off-by: Manik2708 <[email protected]>
Signed-off-by: Manik2708 <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6500 +/- ##
==========================================
- Coverage 96.27% 96.19% -0.09%
==========================================
Files 372 374 +2
Lines 21282 21504 +222
==========================================
+ Hits 20490 20686 +196
- Misses 605 628 +23
- Partials 187 190 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
| @@ -16,7 +16,7 @@ import ( | |||
| "github.com/jaegertracing/jaeger/pkg/es/client" | |||
| ) | |||
|
|
|||
| func newESClient(endpoint string, cfg *Config, tlsCfg *tls.Config) client.Client { | |||
| func NewESClient(endpoint string, cfg *Config, tlsCfg *tls.Config) client.Client { | |||
There was a problem hiding this comment.
why can we not use ES client from pkg/es?
There was a problem hiding this comment.
So are you saying to use the olivere client instead of those used in cmd/es-rollover?
There was a problem hiding this comment.
not olivere client, but our internal abstraction from pkg/es
There was a problem hiding this comment.
not olivere client, but our internal abstraction from pkg/es
But then should I copy this function in factory?
There was a problem hiding this comment.
I suggest you step away from the main issue for now and think about if we need to do a bit of refactoring of the existing code first. I see no reason why all the cmd/es-*** would have to be creating new HTTP client and doing some bespoke stuff that is completely independent of how es storage implementation is written. Everything should be going through pkg/es abstraction.
There was a problem hiding this comment.
Ok then, I could think of these steps on refactoring:
- Refactoring the configs of
rolloverandlookback, encorporating them into the single common config ofes-rollover. This will be very helpful while integrating the rollover with jaeger binary. - As said by you changing the http clients to our abstraction (
pkg/es). - Changing the
UnitandUnitCountto a single parametertime.Duration.
I am thinking of seperate PRs for each. For 1st point, if this suggestion looks good to you, I will change accordingly in the PR [refactor] Seperated the parts of ES-Rollover Config common with ES Config #6516
There was a problem hiding this comment.
yes separate PRs sounds good
| "github.com/jaegertracing/jaeger/pkg/testutils" | ||
| ) | ||
|
|
||
| const ( |
There was a problem hiding this comment.
what is the reason to delete so much code?
There was a problem hiding this comment.
We need to use these tests in jaeger v2 so as to test the auto rollover cron job, therefore we need to refactor these tests. No e2e test is removed in this PR, rather they are changed to use them in v2. I could create a different PR for this but then I had to test the auto rollover e2e, that's why I included it in this PR!
| @@ -24,7 +24,7 @@ for dir in $*; do | |||
| invalid=0 | |||
| for test in ${testFiles}; do | |||
| if grep -q "TestMain" "${test}" && grep -q "testutils.VerifyGoLeaks" "${test}"; then | |||
| if [ "${dir}" != "./plugin/storage/integration/" ] && grep -q "testutils.VerifyGoLeaksForES" "${test}"; then | |||
| if [ "${dir}" != "./plugin/storage/integration/" ] && [ "${dir}" != "./cmd/jaeger/internal/integration/" ] && grep -q "testutils.VerifyGoLeaksForES" "${test}"; then | |||
There was a problem hiding this comment.
if you are introducing leaks you need to fix them, not tweak the linter
There was a problem hiding this comment.
These are not from v2 tests or cron job but the es-rollover tests use the v1 es-rollover tests which uses the official es-clients which are responsible for these leaks. Please see the comments of cmd/jaeger/internal/integration/package_test.go.
Signed-off-by: Manik2708 <[email protected]>
Signed-off-by: Manik2708 <[email protected]>
|
Closing per #6283 (comment) |
Which problem is this PR solving?
Fixes a part of: #6283
Description of the changes
How was this change tested?
Checklist
jaeger:make lint testjaeger-ui:npm run lintandnpm run test