removing the deltafifo to simplify the informer code #3061

shawkins · 2021-04-29T14:31:53Z

Description

The deltafifo adds a lot of complexity to the informer implementation with little to no benefit - it could only combine some delete events. It would be more straight-forward to just act directly over the cache. This reduces the number of threads dedicated to an informer by 1.

It's not in these changes, but it will allow the cache interface to become fully type safe

Type of change

Bug fix (non-breaking change which fixes an issue)
Feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change
Chore (non-breaking change which doesn't affect codebase;
test, version modification, documentation, etc.)

Checklist

Code contributed by me aligns with current project license: Apache 2.0
I Added CHANGELOG entry regarding this change
I have implemented unit tests to cover my changes
I have added/updated the javadocs and other documentation accordingly
No new bugs, code smells, etc. in SonarCloud report
I tested my code in Kubernetes
I tested my code in OpenShift

centos-ci · 2021-04-29T14:31:55Z

Can one of the admins verify this patch?

shawkins · 2021-04-29T15:32:50Z

I don't agree with the sonarcloud bugs - seems like a valid usage of capturing arguments.

I can clear out the type related code smells on a follow-on pr to refine the Cache interface.

shawkins · 2021-04-30T15:16:58Z

I'll rebase this after #3055 is committed

rohanKanojia · 2021-04-30T15:35:28Z

I like this change. I think it'll simplify the flow a lot. Please note that with this we're diverging from client-go's notification distribution mechanism where it pops item from DeltaFifo and processes it.

https://github.com/kubernetes/client-go/blob/master/tools/cache/controller.go#L181

manusa · 2021-04-30T15:39:14Z

I think that many of the low-level implementations that were initially ported from the Go Shared Informers made sense from a Golang perspective but not in a Java word.
I haven't had the chance to review this in depth, but as long as the user/consumer's behavior is more or less compliant with that of Client Go, it should be OK.

shawkins · 2021-04-30T15:50:06Z

I haven't had the chance to review this in depth, but as long as the user/consumer's behavior is more or less compliant with that of Client Go, it should be OK.

From a consumer's perspective nothing meaningful changes. The deltafifo is acting as an intermediary between the watch events and the cache updates / event distribution. However the cost of updating the deltafifo should if roughly the same as an operation directly on the cache (similar global lock, several hash lookups, etc.) - and the end event distribution is already happening on a different thread with the SharedProcessor. The deletefifo is not combining events in any meaningful way. It's only a narrow check for duplicate deletion, which should be handled anyway by the removal of the object from cache by the first delete. So I can't see much value in it currently and it holds a thread.

manusa · 2021-05-05T13:19:53Z

Hi @shawkins
I think you might haven't noticed, but this PR has now conflicts.

shawkins · 2021-05-05T13:26:39Z

I think you might haven't noticed, but this PR has now conflicts.

I can update it if you want. My understanding was this may not go in anytime soon, so I'm not sure how often I need to rebase it.

manusa · 2021-05-05T13:28:49Z

Relates to: #2010

manusa · 2021-05-05T13:31:22Z

My understanding was this may not go in anytime soon, so I'm not sure how often I need to rebase it.

OK sorry, I must be missing something. Why shouldn't this be merged?

shawkins · 2021-05-05T13:33:53Z

That was from a side conversation with @metacosm about not holding up the 5.4 release. I'll go ahead an update it.

manusa · 2021-05-05T13:36:31Z

That was from a side conversation with @metacosm about not holding up the 5.4 release. I'll go ahead an update it.

I think I just read this in the Java Operators GChat.

The idea is to release 5.4 with not too much delay. I wanted to include most of the Informer improvements, and the required generator fixes. Since the latter need an updated version of Sundr.io I think we might be looking into next week for the 5.4 release.

rohanKanojia · 2021-05-05T13:59:07Z

test failure doesn't seem related to this PR:

Error:  io.fabric8.crd.generator.CRDGeneratorTest.checkCRDGenerator  Time elapsed: 0.025 s  <<< FAILURE!
org.opentest4j.AssertionFailedError: expected: not <null>
	at io.fabric8.crd.generator.CRDGeneratorTest.lambda$checkCRDGenerator$5(CRDGeneratorTest.java:224)
	at io.fabric8.crd.generator.CRDGeneratorTest.outputCRDIfFailed(CRDGeneratorTest.java:146)
	at io.fabric8.crd.generator.CRDGeneratorTest.checkCRDGenerator(CRDGeneratorTest.java:219)

[INFO] Running io.fabric8.crd.generator.utils.TypesTest
[INFO] Tests run: 6, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.022 s - in io.fabric8.crd.generator.utils.TypesTest
[INFO] Running io.fabric8.crd.generator.v1.JsonSchemaTest
[INFO] Tests run: 2, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.016 s - in io.fabric8.crd.generator.v1.JsonSchemaTest
[INFO] 
[INFO] Results:
[INFO] 
Error:  Failures: 
Error:    CRDGeneratorTest.checkCRDGenerator:219->outputCRDIfFailed:146->lambda$checkCRDGenerator$5:224 expected: not <null>
[INFO] 
Error:  Tests run: 20, Failures: 1, Errors: 0, Skipped: 0
[INFO] 
[INFO] ------------------------------------------------------------------------

Anyway, I've restarted workflow

metacosm · 2021-05-05T14:36:35Z

This specific issue should go away with the sundrio update (hopefully).

...rnetes-client/src/test/java/io/fabric8/kubernetes/client/informers/cache/ControllerTest.java

manusa

LGTM, thx!

shawkins · 2021-05-06T13:30:28Z

I have one more change I'd like to make here - I'd like to make the replace logic more consistent.

manusa · 2021-05-06T13:43:10Z

I have one more change I'd like to make here - I'd like to make the replace logic more consistent.

I guess we're still waiting for the sundr.io fix, so feel free ;)

this ensures that the cache state is fully updated by the time any of the notifications are processed

shawkins · 2021-05-06T14:03:28Z

Okay this change fully makes use of the underlying stores replace method, and then processes notifications.

sonarqubecloud · 2021-05-06T14:31:14Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
14 Code Smells

82.8% Coverage
0.0% Duplication

…c8io#3061) * removing the deltafifo to simplify the informer code * better reuse of the actual store replace function this ensures that the cache state is fully updated by the time any of the notifications are processed

shawkins force-pushed the remove-deltafifo branch from c32ad76 to 80e2f65 Compare April 29, 2021 14:33

oscerd approved these changes Apr 29, 2021

View reviewed changes

shawkins force-pushed the remove-deltafifo branch from 9b0bde4 to 9624721 Compare April 29, 2021 14:40

manusa requested review from rohanKanojia and manusa April 30, 2021 15:02

shawkins mentioned this pull request Apr 30, 2021

Simplify/consolidate thread utilization by informers #3072

Closed

removing the deltafifo to simplify the informer code

a86357f

shawkins force-pushed the remove-deltafifo branch from 9624721 to a86357f Compare May 1, 2021 11:37

shawkins mentioned this pull request May 4, 2021

refactoring informer logic for type safety and to refine the role of stores #3008

Closed

8 tasks

Merge branch 'master' into remove-deltafifo

2e0e45f

manusa reviewed May 6, 2021

View reviewed changes

...rnetes-client/src/test/java/io/fabric8/kubernetes/client/informers/cache/ControllerTest.java Show resolved Hide resolved

manusa approved these changes May 6, 2021

View reviewed changes

better reuse of the actual store replace function

e201266

this ensures that the cache state is fully updated by the time any of the notifications are processed

shawkins mentioned this pull request May 8, 2021

Refine the Informer Store interface #3091

Closed

Merge branch 'master' into remove-deltafifo

6ca98b6

manusa merged commit 81c5399 into fabric8io:master May 12, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

removing the deltafifo to simplify the informer code #3061

removing the deltafifo to simplify the informer code #3061

shawkins commented Apr 29, 2021

centos-ci commented Apr 29, 2021

shawkins commented Apr 29, 2021

shawkins commented Apr 30, 2021

rohanKanojia commented Apr 30, 2021

manusa commented Apr 30, 2021

shawkins commented Apr 30, 2021

manusa commented May 5, 2021

shawkins commented May 5, 2021

manusa commented May 5, 2021

manusa commented May 5, 2021

shawkins commented May 5, 2021

manusa commented May 5, 2021

rohanKanojia commented May 5, 2021 •

edited

Loading

metacosm commented May 5, 2021

manusa left a comment

shawkins commented May 6, 2021

manusa commented May 6, 2021

shawkins commented May 6, 2021

sonarqubecloud bot commented May 6, 2021

removing the deltafifo to simplify the informer code #3061

removing the deltafifo to simplify the informer code #3061

Conversation

shawkins commented Apr 29, 2021

Description

Type of change

Checklist

centos-ci commented Apr 29, 2021

shawkins commented Apr 29, 2021

shawkins commented Apr 30, 2021

rohanKanojia commented Apr 30, 2021

manusa commented Apr 30, 2021

shawkins commented Apr 30, 2021

manusa commented May 5, 2021

shawkins commented May 5, 2021

manusa commented May 5, 2021

manusa commented May 5, 2021

shawkins commented May 5, 2021

manusa commented May 5, 2021

rohanKanojia commented May 5, 2021 • edited Loading

metacosm commented May 5, 2021

manusa left a comment

Choose a reason for hiding this comment

shawkins commented May 6, 2021

manusa commented May 6, 2021

shawkins commented May 6, 2021

sonarqubecloud bot commented May 6, 2021

rohanKanojia commented May 5, 2021 •

edited

Loading