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

Do not throw error on empty indices in Elasticsach rollover lookback #3369

Merged
merged 6 commits into from
Nov 3, 2021

Conversation

jkandasa
Copy link
Member

@jkandasa jkandasa commented Nov 2, 2021

Signed-off-by: Jeeva Kandasamy jkandasa@gmail.com

Which problem is this PR solving?

Signed-off-by: Jeeva Kandasamy <jkandasa@gmail.com>
@jkandasa jkandasa requested a review from a team as a code owner November 2, 2021 07:47
@@ -54,16 +57,19 @@ func (a *Action) lookback(indexSet app.IndexOption) error {
finalIndices := filter.ByDate(excludedWriteIndex, getTimeReference(timeNow(), a.Unit, a.UnitCount))

if len(finalIndices) == 0 {
return fmt.Errorf("no indices to remove from alias %s", readAliasName)
a.Logger.Info(fmt.Sprintf("no indices to remove from alias %s", readAliasName))
Copy link
Member

Choose a reason for hiding this comment

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

Please pass parameters as zap parameters only. e.g. logger.Info("Message", zap.String("param-name", "foo")

Also prefer to start the log message with a capital letter.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @pavolloffay, updated

Signed-off-by: Jeeva Kandasamy <jkandasa@gmail.com>
Signed-off-by: Jeeva Kandasamy <jkandasa@gmail.com>
pavolloffay
pavolloffay previously approved these changes Nov 2, 2021
@pavolloffay pavolloffay changed the title es rollover lookback, do not throw error on no indices Do not throw error on no indices in Elasticsach rollover lookback Nov 3, 2021
@pavolloffay
Copy link
Member

@jkandasa the test is failing

2021-11-02T14:02:25.6218813Z     --- FAIL: TestLookBackAction/success (0.00s)
2021-11-02T14:02:25.6220046Z panic: runtime error: invalid memory address or nil pointer dereference [recovered]
2021-11-02T14:02:25.6224335Z 	panic: runtime error: invalid memory address or nil pointer dereference
2021-11-02T14:02:25.6225252Z [signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x6705ad]
2021-11-02T14:02:25.6225736Z 
2021-11-02T14:02:25.6226159Z goroutine 7 [running]:
2021-11-02T14:02:25.6226823Z testing.tRunner.func1.2({0x6f08c0, 0x917b80})
2021-11-02T14:02:25.6227807Z 	/opt/hostedtoolcache/go/1.17.2/x64/src/testing/testing.go:1209 +0x36c
2021-11-02T14:02:25.6228563Z testing.tRunner.func1()
2021-11-02T14:02:25.6229486Z 	/opt/hostedtoolcache/go/1.17.2/x64/src/testing/testing.go:1212 +0x3b6
2021-11-02T14:02:25.6230101Z panic({0x6f08c0, 0x917b80})
2021-11-02T14:02:25.6230748Z 	/opt/hostedtoolcache/go/1.17.2/x64/src/runtime/panic.go:1047 +0x266
2021-11-02T14:02:25.6231473Z go.uber.org/zap.(*Logger).check(0x0, 0x0, {0x734fad, 0x17})
2021-11-02T14:02:25.6232224Z 	/home/runner/go/pkg/mod/go.uber.org/zap@v1.19.1/logger.go:268 +0x6d
2021-11-02T14:02:25.6232972Z go.uber.org/zap.(*Logger).Info(0xc00010c0c0, {0x734fad, 0x17}, {0xc0000e2180, 0x2, 0x2})
2021-11-02T14:02:25.6233786Z 	/home/runner/go/pkg/mod/go.uber.org/zap@v1.19.1/logger.go:191 +0x52
2021-11-02T14:02:25.6235206Z github.com/jaegertracing/jaeger/cmd/es-rollover/app/lookback.(*Action).lookback(0xc000087df8, {{0x0, 0x0}, {0x733a5a, 0x13}, {0x731063, 0xb}})
2021-11-02T14:02:25.6236514Z 	/home/runner/work/jaeger/jaeger/cmd/es-rollover/app/lookback/action.go:64 +0x515
2021-11-02T14:02:25.6237660Z github.com/jaegertracing/jaeger/cmd/es-rollover/app/lookback.(*Action).Do(0xc000087df8)
2021-11-02T14:02:25.6239065Z 	/home/runner/work/jaeger/jaeger/cmd/es-rollover/app/lookback/action.go:40 +0x2b6
2021-11-02T14:02:25.6240546Z github.com/jaegertracing/jaeger/cmd/es-rollover/app/lookback.TestLookBackAction.func5(0x0)
2021-11-02T14:02:25.6242109Z 	/home/runner/work/jaeger/jaeger/cmd/es-rollover/app/lookback/action_test.go:145 +0x168
2021-11-02T14:02:25.6242936Z testing.tRunner(0xc0000fe000, 0xc0000273a0)
2021-11-02T14:02:25.6243842Z 	/opt/hostedtoolcache/go/1.17.2/x64/src/testing/testing.go:1259 +0x230
2021-11-02T14:02:25.6244517Z created by testing.(*T).Run
2021-11-02T14:02:25.6245259Z 	/opt/hostedtoolcache/go/1.17.2/x64/src/testing/testing.go:1306 +0x727
2021-11-02T14:02:25.6246373Z FAIL	github.com/jaegertracing/jaeger/cmd/es-rollover/app/lookback	0.021s

@pavolloffay pavolloffay changed the title Do not throw error on no indices in Elasticsach rollover lookback Do not throw error on empty indices in Elasticsach rollover lookback Nov 3, 2021
Signed-off-by: Jeeva Kandasamy <jkandasa@gmail.com>
@codecov
Copy link

codecov bot commented Nov 3, 2021

Codecov Report

Merging #3369 (9859ecc) into master (72f8af5) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3369      +/-   ##
==========================================
- Coverage   96.48%   96.48%   -0.01%     
==========================================
  Files         260      260              
  Lines       15196    15199       +3     
==========================================
+ Hits        14662    14664       +2     
  Misses        451      451              
- Partials       83       84       +1     
Impacted Files Coverage Δ
cmd/es-rollover/app/lookback/action.go 100.00% <100.00%> (ø)
cmd/query/app/server.go 94.11% <0.00%> (-1.48%) ⬇️
pkg/config/tlscfg/cert_watcher.go 94.73% <0.00%> (+2.10%) ⬆️

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 72f8af5...9859ecc. Read the comment docs.

@jkandasa
Copy link
Member Author

jkandasa commented Nov 3, 2021

@pavolloffay I have added a fix for nil exception in unit test
can you please review now?

@pavolloffay pavolloffay merged commit 5b89720 into jaegertracing:master Nov 3, 2021
@jkandasa jkandasa deleted the fix_index_rollover branch November 3, 2021 14:43
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.

ES rollover lookback: do not throw exception on no indices to remove from alias
2 participants