Add rpc server to helm chart

[dstandish]

I call the component "rpc server" beacause the there isn't really just one "internal API" -- the other one is the one used by the webserver.

I think we might want to consider renaming it at the CLI also.

Ideally, also we could somehow mark this (rpc server) private in the helm chart until AIP-44 is "done"

[potiuk]

Agree RPC server is a better name. Also marking it as private (how?) is a good idea.

[dstandish]

Agree RPC server is a better name. Also marking it as private (how?) is a good idea.

Maybe we rename rpcServer section “_rpcServer” until release?

[potiuk]

Why not. It will also be disabled by default so no big deal and we still have the feature flag AIRFLOW_ENABLE_AIP_44 set to False by default that prevents the component from starting.

[dstandish]

Why not. It will also be disabled by default so no big deal and we still have the feature flag AIRFLOW_ENABLE_AIP_44 set to False by default that prevents the component from starting.

Just to confirm.... given that it's behind feature flag anyway, would it be sufficent to just call it out as "private" in the docs? Feels like a stretch. @jedcunningham thought?

[jedcunningham]

Let's prefix with _, or maybe _aip44RpcServer? I'm hesitant of a docs only change.

And yes the component wouldn't start, but it would go into crashloop. That said, it seems like we have a miss on our chart integration tests. A component being stuck in crashloop / never actually running should surely be a failure, no?!

[dstandish]

Let's prefix with _, or maybe _aip44RpcServer? I'm hesitant of a docs only change.

Did it.

Went with _rpcServer. I think that's definitely clear enough.

When we make it "public" then we'll have to change it both at top level and in .Values.ports._rpcServer. But that should be easy to catch since it will show up in a find / replace.

I think this is should be ready to go now, presuming tests pass now, especially now that we made it "private". I added comments and newsfragment.

[vincbeck]

My knowledge in Kubernetes is too limited to review this PR :/ I'll leave it to better eyes than mine

[eladkal]

I think we should have some docs to explain about this capability (and mark it as experimental in the docs as well)

[dstandish]

I think we should have some docs to explain about this capability (and mark it as experimental in the docs as well)

Hi @eladkal

Are you talking about the helm chart docs? Or the regular airflow docs? I looked in helm chart docs and don't see anything analogous for other services. E.g. there's no "triggerer" section or "scheduler" section. Is there a particular place you think an RPC server section would make sense?

[eladkal]

Yes I am talking about the helm chart docs.
Consider someone who came across this rpc feature. The most basic question is what do I need this for? What value does it bring?
If this refrence an Airflow feature that we already have docs on then we just need to link it.

The reason I am raising this is you added newsfragmant that essentially is a new feature.. it's normally accompanied with docs

[dstandish]

Yes I am talking about the helm chart docs. Consider someone who came across this rpc feature. The most basic question is what do I need this for? What value does it bring? If this refrence an Airflow feature that we already have docs on then we just need to link it.

The reason I am raising this is you added newsfragmant that essentially is a new feature.. it's normally accompanied with docs

I understand. Yeah, it's not released yet though. Maybe I should remove the newsfragment? Cus this is just private and for dev use only. So maybe it's the wrong time to add a newsfragment. The AIP is not near complete yet and actually, some of the comments on dev list recently indicate that now with 3.0 on the horizon, we might not actually ever complete AIP-44.

But anyway, typically in helm chart, we don't document the service itself. That's left to the core docs. But the core docs have not yet been written (and reasonably so, I think, because we're not yet very near to completion and especially now with the uncertainty). So I think it's OK to leave undocumented for now. But I will remove the newsfragment because I think that's a reasonable thing to do.

Let me know if you have additional concerns.