Skip to content

Java informer portal implementation #461

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Mar 7, 2019
Merged

Java informer portal implementation #461

merged 3 commits into from
Mar 7, 2019

Conversation

yue9944882
Copy link
Member

@yue9944882 yue9944882 commented Dec 10, 2018 •
edited
Loading

Gopher testing

Plz ignore any funny part in this pull if u notice 🙂Nvmd, i will revert later

Also, am opening a consumer part of this pull at k/gen repo

/hold

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Dec 10, 2018
yue9944882
yue9944882 commented Dec 10, 2018
View reviewed changes
examples/src/main/java/io/kubernetes/client/examples/InformerExample.java Outdated
SharedInformerFactory factory = new SharedInformerFactory();

// Node informer
ClusterScopeInformer<V1Node> nodeInformer = factory.informerForClusterScopeResource(V1Node.class);
Copy link
Member Author

Choose a reason for hiding this comment

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

example stands out here @mbohlool @brendandburns

@yue9944882 yue9944882 mentioned this pull request Dec 10, 2018
Closed
@yue9944882 yue9944882 force-pushed the feature/informer-impl branch from 4f99714 to 5ee6189 Compare December 10, 2018 17:10
@brendandburns
Copy link
Contributor

Thanks for starting this PR!

Can I get a quick doc added that gives a high-level guide to the approach here?

It doesn't seem to me like forking the http-gson code is sustainable (or necessary) but I'm probably not fully thinking about the problem correctly.

@yue9944882 yue9944882 force-pushed the feature/informer-impl branch 2 times, most recently from 94ec012 to 1b0be5b Compare December 13, 2018 15:09
@brendandburns
Copy link
Contributor

So when I had thought about this, I had thought about using Generics and some interfaces to build a single Informer object. I think that it is do-able, and much cleaner than code gen.

Happy to send you my sketch.

@yue9944882
Copy link
Member Author

@brendandburns sure, am all ears. where the link? 👍🏻

@brendandburns
Copy link
Contributor

@yue9944882 Here's the rough sketch:

#468

It's an implementation of a ListWatch cache, but it would be easy to add event handlers to implement a generic Informer....

@yue9944882 yue9944882 force-pushed the feature/informer-impl branch 2 times, most recently from 367bcbd to c3f4ba4 Compare December 25, 2018 06:17
@yue9944882 yue9944882 changed the title [WIP] Java informer portal implementation Java informer portal implementation Dec 25, 2018
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 25, 2018
@yue9944882 yue9944882 force-pushed the feature/informer-impl branch 2 times, most recently from 5be4d59 to 7bf882b Compare December 25, 2018 16:21
yue9944882
yue9944882 commented Dec 26, 2018
View reviewed changes
util/pom.xml
@@ -139,8 +139,8 @@
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-compiler-plugin</artifactId>
<configuration>
<source>1.7</source>
<target>1.7</target>
<source>8</source>
Copy link
Member Author

Choose a reason for hiding this comment

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

lambda is supported since java8

@yue9944882
Copy link
Member Author

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 27, 2018
@yue9944882 yue9944882 force-pushed the feature/informer-impl branch from 7bf882b to 4029c6d Compare December 28, 2018 15:52
@yue9944882 yue9944882 mentioned this pull request Dec 31, 2018
Closed
@yue9944882
Copy link
Member Author

@brendandburns here's a brief note about our informer

https://docs.google.com/document/d/1HFOcs39oTD3g-cBuWyLd5QaevPg6omCBWnCbPV5JvBg/edit

@yue9944882
Copy link
Member Author

politely bumping this thread @brendandburns

@brendandburns
Copy link
Contributor

Sorry, didn't realize this PR was still active, I will review ASAP...

brendandburns
brendandburns reviewed Feb 28, 2019
View reviewed changes
util/src/main/java/io/kubernetes/client/informer/cache/Deltas.java Outdated
}
}

public enum DeltaType {
Copy link
Contributor

Choose a reason for hiding this comment

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

This feels like a duplicate of EventType?

Copy link
Member Author

@yue9944882 yue9944882 Mar 4, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

no, it isn't. delta type represents an inner state of the queuing items which has one more type "Sync" comparing to event type. while event type is exposed as a part of the informer's public API.

@brendandburns
Copy link
Contributor

Many thanks for the tests! I added a few more comments about reducing the number of custom classes in the implementation so that it's easier to understand.

@yue9944882 yue9944882 force-pushed the feature/informer-impl branch from 07e30df to 7c3533e Compare March 4, 2019 12:59
@brendandburns
Copy link
Contributor

Thanks for all the changes and the patience. Add the TODO and this is LGTM and ready to merge.

@adohe-zz
Copy link
Contributor

adohe-zz commented Mar 5, 2019

so glad to see this get merged @yue9944882 @brendandburns as a follow up we could help build more utility, like work-queue, leader election. Let's just get this Java informer framework fully functional.

@yue9944882 yue9944882 force-pushed the feature/informer-impl branch from 7c3533e to 296e569 Compare March 5, 2019 05:14
@brendandburns
Copy link
Contributor

@adohe do you want me to wait on an LGTM from you before I merge? Otherwise, I think this is good to go...

@adohe-zz
Copy link
Contributor

adohe-zz commented Mar 6, 2019

overall lgtm. Let's just get it merge @brendandburns

@brendandburns
Copy link
Contributor

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 7, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: brendandburns, yue9944882

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 7, 2019
@k8s-ci-robot k8s-ci-robot merged commit f1e5a04 into kubernetes-client:master Mar 7, 2019
@brendandburns brendandburns mentioned this pull request Mar 7, 2019
Closed
@yue9944882 yue9944882 mentioned this pull request Mar 7, 2019
Closed
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adohe-zz adohe-zz adohe-zz left review comments

@brendandburns brendandburns brendandburns left review comments

@mbohlool mbohlool Awaiting requested review from mbohlool

Assignees

@brendandburns brendandburns

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@yue9944882 @brendandburns @adohe-zz @k8s-ci-robot