-
Notifications
You must be signed in to change notification settings - Fork 7
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
Add page on core developers conversations #20
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,27 @@ | ||||||||||
--- | ||||||||||
--- | ||||||||||
# Core developers conversations | ||||||||||
|
||||||||||
Core developers meet on a semi regular schedule to discuss | ||||||||||
the code being contributed; here is a summary of the points discussed, | ||||||||||
and a summary of the level of consensus reached. | ||||||||||
|
||||||||||
**[C]** means that there is consensus on the bullet point; otherwise it | ||||||||||
means the point has been discussed but no final decision has been made | ||||||||||
|
||||||||||
|
||||||||||
## 2024-01-16 | ||||||||||
|
||||||||||
We spoke about the AsyncCalls and their frameworks and implementations, | ||||||||||
and filesystem layout. This conversation stemmed from | ||||||||||
[PR 1635](https://github.com/squid-cache/squid/pull/1635) and | ||||||||||
[PR 1548](https://github.com/squid-cache/squid/pull/1548). | ||||||||||
|
||||||||||
* **[C]** in this conversation we are not concerned with the mechanisms by which individual calls are fired | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why say this? We are not concerned with a lot of things. What is so special about the firing mechanism? Does the firing mechanism include the concept of firing the call away from (and at a different time than) call creation? If it does, then there is no consensus regarding this statement.
Suggested change
|
||||||||||
* **[C]** AsyncCall is a delayed function call, that can change the stack | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why are we describing what AsyncCall is? AsyncCall is an existing class. If our understanding of that class differs from that class description, we can document that, but that requires a rather different statement (and, ideally, we should just fix the class documentation instead). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does a C++ function pointer represent an asynchronous call concept as well? Like AsyncCall, a function pointer is used to delay the function call and, when used/fired/called, it (like any function call) "changes the stack". Either there is something missing in this attempt to define asynchronous call concept OR we should not tie that definition to the AsyncCall class so much. |
||||||||||
* **[C]** our strategic goal is that all call sthat cannot be expressed as a c++ function call be implemented as AsyncCalls, including all callbacks | ||||||||||
* **[C]** call queues and lists and so on are logically tied to calls, not to the firing mechanisms | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Some call collections may be tied to the firing mechanisms as well.
Suggested change
|
||||||||||
* AsyncCall.h and related files should be moved out of `src` and into `src/calls` | ||||||||||
* @kinkie will prepare a draft proposal on the things to be moved. | ||||||||||
The proposal should include a high level idea about why things should be moved; | ||||||||||
it does not need to include any description of why other things should not be moved | ||||||||||
Comment on lines
+26
to
+27
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO, the current CoreDevConvos page intent (as implied by the above description) would be too costly to support and will become stale/abandoned. For example, it may take us several weeks to polish and merge
[C]
bullet points; by that time, even if we end up with something more useful than a couple of obvious facts, we will be a meeting behind. Also, meeting date is a poor choice for the primary key because establishing consensus often takes more than one meeting and often takes place outside of a meeting (e.g., this PR reviews); we should be focusing on concepts rather than their live discussion schedule.Instead of a diary, we should maintain a collection of things we agree on (including, in some special cases, things we agree to disagree on). Git log and GitHub history can always be used to map that collection to a diary if anybody really needs the latter.
FWIW, I have proposed to add such a collection of notes and even bootstrapped it at https://github.com/measurement-factory/squid-notes/blob/start/README.md. I suggest refactoring the proposed docs/RoadMap/CoreDevConvos.md page as a page dedicated to asynchronous calls in Squid and moving this page into a new docs/notes directory (where other notes like that can be accumulated, debated, and maintained).