Architecture meetings/Amsterdam Hackathon 2013 Day 1

Amsterdam Hackathon 2013 Day 2

Meeting at Amsterdam Hackathon 2013

Date: 2013-05-24

Attendees: Adam B, Jeroen, Daniel W, Yuri Roan, Daniel K, Gabriel, Brion, Max, Niklas, Tim, Chris, Brad, Katie, Siebrand, Andrew, Derk-Jan, Antoine, Subbu

Preamble

For example, regarding return values.. When you create a Title object and it considers the input string name to be invalid, there is no exception, instead the constructor will return null instead of a Title object. On success it has a Title object so it's attractive/easy to forget (until it expodles in your face).

Tim/Brion: Status objects force one to think about whether there may be an error (Status::getValue, Status::isOK, Status::isGood)

But they're overhead and somewhat uncomfortable to use everywhere (what would be the limit, when to and when not to use Status).

Timo: A similar dilemma exists in some of our js projects where we return a lot of Promise objects without proper error handling. Or in nodejs where you keep stamping: "function (err, data) { if (err) { throw err; } ...data... }".

Exceptions wouldn't be appropiate in case of Title since an invalid Title (or rather invalid input to it) does not constitute an application error.

   # Current
   $t = Title::newFrom..( ..);
   if (!$t) { .. }
   $t->stuff // blows up with non-object without the if check above

   # Title object accounting for invalid data?
   $t = Title::newFrom..( ..);
   if ( !$t->isValid( .. ) ) { .. }
   $t->getNamespace() // akward method and prone to error since method should assume title is valid and actually has a namespace

   # Require check first?
   if ( !Title::isValid( ... ) ) { ... }
   $t = Title::newFrom..( .. );  // may throw if invalid, user expected to do the isValid first
   $t->stuff; // works

.. let's revisit later

... moving on, to Daniel Kinzler's slidedeck

Daniel's slides

Mixing concerns

DAO code mixed into value objects (Title, Revision, etc)
UI code in value objects (Content class)
UI code mixed with controller code (EditPage)

Idea: Should we use the API as our logic and use it from the ("other") end points (index.php, maintenance scripts). Or have separate classes and treat both API, index.php and maintenance scripts as a thin layer.

PrefixIndex is a good example of separate class (used from both API and SpecialPage). Tim: ApiEdit is a good example of how not to do it (use the frontend class in the API). Using API elsewhere currently requires faking a request (just like ApiEdit does for EditPage, so the other way around has the same problem).

> Separate classes are nice. Reduces need to "test" both end points. Instead you test the individual logic classes and the end points themselves (API test, index.php test, test for PrefixIndex, we shouldn't need to test ApiPrefixIndex or SpecialPrefixIndex)

Global state

Config
$wgUser, etc (Context)
Static services (wfGetDB)
Caching using static variables (Namespaces)
- Caching should be data access layer? what about computed values?
- Static variables may be necessary to deal with legacy code
Hard to test

Kinds of classes

Values
Services
Controllers
Misc. (glue code)

Any class should do only one of these, currently our classes often mix two or even three of these.

What I've seen is that we can't batch things because if objects control their own state, you can't deal with multiple instances. Brion: We have some of this the media handling, but it gets muddled.

Aspects

Business logic
Rendering
HTTP Request/Response
Storage

Again, classes should relate to only one of these four.

(De)compose

Up side:

testability
reusability
documentation
clear responsibilities and dependencies

Down side:

many classes
injection interface (constructor)
$fooStore->save($foo) instead of $foo->save()

Gabriel: Should $foo have a reference to $fooStore and have $foo->save call $fooStore-save($this)? Roan/Timo: We tried that in VE, and ran into problems. The value would need to know about the storage (either save fails if not yet given, or it becomes required in the constructor)

Better separation of concerns, easier to test, storage-agnostic value-related only, easy to serialize regardless of storage.

... end of slides.

Hooks

Tim: What do we think about Hooks? There are viable alternatives.

Interface objects. How would that work?

Misc points

Avoid big commits and people complaining later.
Avoid small steps without a clear vision.
We'll slowly go from "Let's do new stuff like this and gradually migrate/deprecate the rest" to "Reject code that doesn't comply"
(Yuri:) Enforce it during code review. I like how we do it now for coding style. Reference the code conventions manual, no personal defence but refer to the document. Don't fight the reviewer, fight the document.

Gabriel: Identify potential services with narrow interfaces, for example for the storage layer.

Yuri: Many sites (e.g Google) use multi-layered systems. Different isolated services/servers talking to each other.

Daniel: WikiPage "save" is more than just storage. There is also logic (user rights for example).

Meta talk about format

Title: "Architecture Guidelines"
Alt.: "Architecture Recommendation"
Alt.: "Architecture Design"

Tim: We don't really use wikis like a wiki very often.

Brion: We often do a combination of wiki, mailing lists, etherpad, google docs.

Tim: c2.com wiki-style essay documents on how things should be done

Robla: I'm willing to get the document started (forming the committee). Who would want to be on it?

Antoine Musso
Aude
Brion Vibber
Daniel Kinzler
Daniel Werner
Gabriel Wicke
Jeroen De Dauw
Max Semenik
Niklas Laxström
Roan Kattouw
Siebrand Mazeland
Tim Starling
Timo Tijhof
Yuri Astrakhan

Yuri: How far do we go? People have written entire books about this. We can't fit all that in a simple document.

Robla: We'll be making trade offs where needed. We will be making choices and discard options that may be viable, too.

Tim: Books not freely available to the community. We should have our own rationales (not just what to do, but also why)

Brion: Book is the source code. Our document will be our point of reference.

Chris: We need committee members to mentor (i.e. review code)

Robla: We we'll start by creating a rough draft on the wiki. From there we can take topics on the talk page and reach out via the mailing list for major changes or addititions.

Meta talk about content

Timo: Let us keep this in the back of our hands the coming days/weeks during Code Review and keep track of things of good/bad patterns we encounter. After a short while we'll have a good chunk of real use cases and clearly identifiable areas that need work (or are examples of what we did right).

Brad: When I look up the terms from Daniel's slides, what I find makes little sense to me. I didn't get buzz words in school.

Siebrand: Make sure we clearly document our terminology and apply it consistenty.

Yuri: Have some people who are senior with the code add various "@todo FIXME" comments in the codes. Flags for other people to learn from and address it. Concentrate on extensions as they tend to be less looked at, and are more likely to break when core is changed.

Siebrand: We have lots of them (red. @todo comments) already. Not often addressed?

Timo: Antoine and I recently upgraded Doxygen. Timo fixed the @todo comments in core to all match the "@todo" pattern (TODO:, FIXME: @TODO etc.). We now have a Todo Index that's generated with the rest of php documentation post-merge on each commit. Go bunkers!

https://doc.wikimedia.org/mediawiki-core/master/php/html/todo.html

Meta talk about time table

Siebrand: When are we going to have the first actionable version?

Daniel: It's good to have a timeline. e.g. good in 6 weeks? (something usable, not unchangable)s

Robla: By the end of this week, have a respectable draft.

Siebrand: +1

Brion: Next checkpoint: a few months from now. Say, by or at Wikimania (August 2013)

Siebrand: Prepare the community (extension developers) about what we're (about to be) doing.

Daniel: At the wikimania hackathon, have a showcase for what and why we made these decisions (once we made them).

Siebrand: Next Wikimania is in Hong Kong, maybe not the ideal location to use as a podium for this – considering the people involved with this subject and travel distance to Hong Kong.

We'll evaluate at Hong Kong, but discussion stays in public channels (mailng list) and progress reflected on the draft on the wiki page

Future meetings

Robla: Let's discuss more often like this. Mailing lists, wiki page / wiki talk page, IRC meetings?

Robla: We're out of time for this discussion. We'll be here for 2 more days. We might be able to get the room back again for another session. Will we have more meetings like these in future? It's too big for a hangout.

Robla: Next session after lunch tomorrow.

Brion: We'll have tomorrow. For future evaluation let's make it public: "Mailing list or wiki, or it didn't happen."

Daniel: Topic for tomorrow (or tonight): Refactoring an API module: ApiQueryLangLinks

https://gerrit.wikimedia.org/r/#/c/60034/

→ Amsterdam Hackathon 2013/Architectural principles document/Day 2

See also