[AIRFLOW-6416] Sort default connection by conn_id

[zhongjiajie]

• Remove unnecessary annotation provide_session
• Sort create_default_connections by connection id
• Add sort connection id unittest

• Description above provides context of the change
• Commit message contains [AIRFLOW-XXXX] or [AIRFLOW-6416] for document-only changes
• Unit tests coverage for changes (not needed for documentation changes)
• Commits follow "How to write a good git commit message"
• Relevant documentation is updated including usage instructions.
• I will engage committers as explained in Contribution Workflow Example.
  In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
  In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
  In case of backwards incompatible changes please leave a note in UPDATING.md.
  Read the Pull Request Guidelines for more information.

[zhongjiajie]

Oh, we already add more probot to repo, Mergeable is good!

[codecov-io]

Codecov Report

Merging #6987 into master will decrease coverage by 0.28%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master   #6987      +/-   ##
=========================================
- Coverage   84.99%   84.7%   -0.29%     
=========================================
  Files         679     679              
  Lines       38647   38647              
=========================================
- Hits        32847   32737     -110     
- Misses       5800    5910     +110

Impacted Files	Coverage Δ
airflow/utils/db.py	100% <100%> (ø)	⬆️
airflow/kubernetes/volume_mount.py	44.44% <0%> (-55.56%)	⬇️
airflow/kubernetes/volume.py	52.94% <0%> (-47.06%)	⬇️
airflow/kubernetes/pod_launcher.py	45.25% <0%> (-46.72%)	⬇️
airflow/kubernetes/refresh_config.py	50.98% <0%> (-23.53%)	⬇️
...rflow/contrib/operators/kubernetes_pod_operator.py	78.75% <0%> (-20%)	⬇️

---

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 869ca96...ccaa6d8. Read the comment docs.

[potiuk]

@zhongjiajie why do you want to remove @provide_session ? The way it works now, it will create a new session for each connection - each merge_conn will have a new session (which basically means new connection established with the database). With @provide_sesion, the session will be reused whereas with the new approach it will be create/commit/close for every single Connection entry.

[potiuk]

We should not remove @provide_session as it is there to make all the connections reuse session rather than recreate sessions.

[zhongjiajie]

And we could not remove @provide_session in function merge_conn due to some unittest use it to create temp connections, do we? @potiuk

[zhongjiajie]

@zhongjiajie why do you want to remove @provide_session ? The way it works now, it will create a new session for each connection - each merge_conn will have a new session (which basically means new connection established with the database). With @provide_sesion, the session will be reused whereas with the new approach it will be create/commit/close for every single Connection entry.

I think we don't need to create connect in function create_default_connections, because it will be create in function merge_conn, so I remove it. but when you comment in

We should not remove @provide_session as it is there to make all the connections reuse session rather than recreate sessions.

I get that point

[potiuk]

And we could not remove @provide_session in function merge_conn due to some unittest use it to create temp connections, do we? @potiuk

The @provide_session decorator (and merge_conn uses it) follow exactly the pattern of session/transaction passing. It's there to handle the case that a session is created once, transaction starts, and you reuse the session do everything in the same DB transaction. So it is really needed :)

[kaxil]

Do we need this? The connections in the Airflow UI are already sorted.

[zhongjiajie]

I think it maybe more easy to maintain the code, what do you think? @kaxil

[kaxil]

I think it maybe more easy to maintain the code, what do you think? @kaxil

I don't have a strong opinion against it as you might be right it might be good to have it in alphabetical order. WDYT @potiuk ?

[potiuk]

No strong opinion - but I think it only makes sense if we enforce it afterwards (like we did with a number of other pre-commit checks). It's low priority I think.

[zhongjiajie]

So what should I do in this situation ? close this PR or do some else? @potiuk @kaxil

[potiuk]

I like that you added test for it :)

[zhongjiajie]

Thanks Jarek and Kaxil for review