Add a quick tour of the project to CONTRIBUTING #30187

nik9000 · 2018-04-26T21:27:49Z

Adds a description of the most important subdirectories to
CONTRIBUTING.md to help folks that are less familiar with the project
get their bearings. It reflects that state of the project right now so
it will inevitably go out of date. But I'll try and keep it up to date.

Adds a description of the most important subdirectories to `CONTRIBUTING.md` to help folks that are less familiar with the project get their bearings. It reflects that state of the project right now so it will inevitably go out of date. But I'll try and keep it up to date.

elasticmachine · 2018-04-26T21:27:51Z

Pinging @elastic/es-core-infra

nik9000 · 2018-04-27T11:29:55Z

@elasticmachine, retest this please

cbuescher

Great addition, I think an overview over the project structure like this is very helpful for contributors. I left a few comments, mostly matters of taste, please treat these as suggestions and not as "hard" change requests. Would like to hear your opinion on these.

cbuescher · 2018-04-27T14:38:49Z

CONTRIBUTING.md

+        [semver](https://semver.org/) their APIs or accept feature requests
+        for them. We publish them to maven central because they are
+        dependencies of our plugin test framework, high level rest client, and
+        jdbc driver but.


Dangling "but"? Somethings is missing or too much here it seems.

cbuescher · 2018-04-27T14:40:59Z

CONTRIBUTING.md

+This repository is split into many top level directories. The most important
+ones are:
+* docs: Documentation for the project.
+* distribution:  Builds our tar and zip archives and our rpm and deb packages.


I'm on the fence with the "we"/"our" style in this case, maybe this (and the rest of this description) could be rephrased in an neutral style, here e.g. "builds the tar and zip archive ...". I haven't checked how this fits into the rest of the Contributing docs though, so please ignore if this doesn't fit.

I tend to use we/our/you instead of the more declarative style. I figure it makes us sound like we're humans talking to humans rather than corporate robots. I'm not 100% consistent with it but I try. We have a long history of having folks write in their own voice, so you get some stuff in this style by me and others in a super formal, passive voice style.

cbuescher · 2018-04-27T14:41:35Z

CONTRIBUTING.md

@@ -209,6 +209,72 @@ Before submitting your changes, run the test suite to make sure that nothing is
 ./gradlew check
 ```

+### Project layout
+
+This repository is split into many top level directories. The most important


"consists of" vs. "split"? Maybe not that important.

I like to pick shorter words.

cbuescher · 2018-04-27T14:42:05Z

CONTRIBUTING.md

+        for them. We publish them to maven central because they are
+        dependencies of our plugin test framework, high level rest client, and
+        jdbc driver but.
+* modules:  Features that are shipped with Elasticsearch by default but are not


Maybe add "Contains features..."

cbuescher · 2018-04-27T14:43:42Z

CONTRIBUTING.md

+        because they require permissions that we don't believe *all* of
+        Elasticsearch should have or because they depend on libraries that we
+        don't believe *all* of Elasticsearch should depend on. For example,
+        reindex requires the `connect` permission so it can perform


The examples are interesting, but maybe not that important for an overview. Just my 10 cents.

cbuescher · 2018-04-27T14:43:59Z

CONTRIBUTING.md

+        Elasticsearch should have the "connect". For another example, Painless
+        is implemented using antlr4 and asm and we don't believe that *all*
+        of Elasticsearch should have access to them.
+* plugins:  Officially supported plugins to Elasticsearch. We decide that a


Contains...

This is another example of where I'd prefer something other than "we" (and with that in the rest of the list here too). I'm not even sure exactly why, probably because it creates this "we" vs. "you" feeling that I think might sound a bit alienating for external contributors? I might be completely off here, let me know your thoughts.
A way to phrase this differently might be e.g. "A feature is deemed best to be a plugin ... if it seems only important..."

Hmmm. I hadn't thought of the "we" vs "you" feeling. I mean, I don't want people to feel excluded. In some sense this is "we, the maintainers" vs "you, the contributor who may or may not want to become a maintainer" and "we" vs "you" is accurate. I'll think about it!

I don't like your way because removing the actors makes it seem like the author of the document thinks that the logic is "universal" or "obvious" or something like that. But it isn't. We chose to do the things we chose and it'd have been fairly reasonable to chose otherwise.

I understand your point. I will think more about what seems to be bothering me here (reaction to language is interesting I think) but of course we can leave it like it is for this PR.

cbuescher · 2018-04-27T14:51:16Z

CONTRIBUTING.md

+        canonical example of this is the ICU analysis plugin. It is important
+        for folks who want the fairly language neutral ICU analyzer but the
+        library to implement the analyzer is 11MB.
+* qa:  Honestly this is kind of in flux and we're not 100% sure where we'll end


As a matter of taste, I'd also prefer it this has less of a "conversational" feel to it. Same vain as in the "we" case.

I think being conversational is very important when we really don't know the right answer. I'm not sure I could be both formal and unsure. And I think it is important to communicate how we're unsure here.

cbuescher · 2018-04-27T14:52:32Z

CONTRIBUTING.md

+            disabled
+    * Tests that need to do strange things like install plugins that thrown
+            uncaught `Throwable`s or add a shutdown hook
+        But we're not convinced that all of these things *belong* in the qa


Not sure what the intentions are here, but maybe these justifications for the current state could be left out?

I think they are important.

cbuescher · 2018-04-27T14:53:26Z

CONTRIBUTING.md

+        you'll get several opinions.
+* server:  The server component of Elasticsearch that contains all of the
+        modules and plugins. Right now things like the high level rest client
+        depend on the server but we'd like to fix that in the future.


Another suggestion of how you might avoid "we": "there are plans to fix..." would work well I think.

cbuescher

Hi Nik, thanks for the replies. I'm the last one who wants to prevent anybody from using their own voice over a more passive voice style, just something I wanted to discuss. I left a few follow up comments regarding formatting and a few minor nits.

cbuescher · 2018-04-28T11:11:51Z

CONTRIBUTING.md

+We're fairly sure that "tests that require multiple modules or plugins to work"
+should just pick a "home" plugin. We're fairly sure that the multi-version
+tests *do* belong in qa. Beyond that, we're not sure. If you want to add a new
+thing qa module, ask around and you'll get several opinions.


Something missing "a new thing qa module"

Maybe "ask around" needs some more clarification? For internal users this would be Slack, but otherwise probably Github? The Forum? Not sure if we have a prefered point of contact. If somebody wants to add sth. its probably a PR anyway?

I'll reword it mentioning PRs, yeah.

cbuescher · 2018-04-28T11:13:28Z

CONTRIBUTING.md

+We publish them to maven central because they are dependencies of our plugin
+test framework, high level rest client, and jdbc driver but they really aren't
+general purpose enough to *belong* in maven central. We're still working out
+what to do here....


Maybe ommit the dots.

cbuescher · 2018-04-28T11:22:19Z

CONTRIBUTING.md

+Right now the directory contains
+* Tests that require multiple modules or plugins to work
+* Tests that form a cluster made up of multiple versions of Elasticsearch like
+        full cluster restart, rolling restarts, and mixed version tests


When rendered e.g in Atom, the indentations make these line appear in boxes in a fixed font, just like code examples.
I don't think thats the intention here.

Not at all. VSCode's preview doesn't do that. I'll remove the indentation anyway.

cbuescher · 2018-04-28T11:23:09Z

CONTRIBUTING.md

+* Tests that need to do strange things like install plugins that thrown
+        uncaught `Throwable`s or add a shutdown hook
+But we're not convinced that all of these things *belong* in the qa directory.
+We're fairly sure that "tests that require multiple modules or plugins to work"


Why the quotes?

They made sense in my head the first time. They don't make sense this time around though. Dropping them.

cbuescher · 2018-04-28T11:27:18Z

CONTRIBUTING.md

+#### `test`
+Our test framework and test fixtures. We use the test framework for testing the
+server, the plugins, and modules, and pretty much everything else. We publish
+the test framework so folks who develope Elasticsearch plugins can use it to


I just learned sth. new about develope vs. develop (https://en.wiktionary.org/wiki/develope)

That was just a spelling mistake!

nik9000 · 2018-04-30T15:41:35Z

@cbuescher I cleaned it up a little more, mostly just covering things you pointed out. Would you care to have another look?

cbuescher · 2018-05-02T08:41:07Z

CONTRIBUTING.md

@@ -269,7 +269,7 @@ disabled
 * Tests that need to do strange things like install plugins that thrown
 uncaught `Throwable`s or add a shutdown hook
 But we're not convinced that all of these things *belong* in the qa directory.
-We're fairly sure that tests that require multiple modules or plugins to work
+We're fairly sure that tests that require multiple modules or plugins to work`


nit: how did the backtick sneak in? Does it to anything?

cbuescher · 2018-05-02T08:46:26Z

CONTRIBUTING.md

+* Tests that need to do strange things like install plugins that thrown
+uncaught `Throwable`s or add a shutdown hook
+But we're not convinced that all of these things *belong* in the qa directory.
+We're fairly sure that tests that require multiple modules or plugins to work`


nit: how did the backtick sneak in? Does it to anything?

Indeed. Typo.

cbuescher

@nik9000 thanks, I found another smallish typo (maybe?) but the rest looks great now. I think its ready once you looked at the last comment I left.

Adds a description of the most important subdirectories to `CONTRIBUTING.md` to help folks that are less familiar with the project get their bearings. It reflects that state of the project right now so it will inevitably go out of date. But I'll try and keep it up to date.

nik9000 · 2018-05-07T23:44:11Z

Thanks for reviewing @cbuescher! I've merged to master and cherry-picked to 6.x.

Adds a description of the most important subdirectories to `CONTRIBUTING.md` to help folks that are less familiar with the project get their bearings. It reflects that state of the project right now so it will inevitably go out of date. But I'll try and keep it up to date.

* master: Mute ML upgrade test (#30458) Stop forking javac (#30462) Client: Deprecate many argument performRequest (#30315) Docs: Use task_id in examples of tasks (#30436) Security: Rename IndexLifecycleManager to SecurityIndexManager (#30442) [Docs] Fix typo in cardinality-aggregation.asciidoc (#30434) Avoid NPE in `more_like_this` when field has zero tokens (#30365) Build: Switch to building javadoc with html5 (#30440) Add a quick tour of the project to CONTRIBUTING (#30187) Reindex: Use request flavored methods (#30317) Silence SplitIndexIT.testSplitIndexPrimaryTerm test failure. (#30432) Auto-expand replicas when adding or removing nodes (#30423) Docs: fix changelog merge Fix line length violation in cache tests Add stricter geohash parsing (#30376) Add failing test for core cache deadlock [DOCS] convert forcemerge snippet Update forcemerge.asciidoc (#30113) Added zentity to the list of API extension plugins (#29143) Fix the search request default operation behavior doc (#29302) (#29405)

* 6.x: Stop forking javac (#30462) Fix tribe tests Docs: Use task_id in examples of tasks (#30436) Security: Rename IndexLifecycleManager to SecurityIndexManager (#30442) Packaging: Set elasticsearch user to have non-existent homedir (#29007) [Docs] Fix typo in cardinality-aggregation.asciidoc (#30434) Avoid NPE in `more_like_this` when field has zero tokens (#30365) Build: Switch to building javadoc with html5 (#30440) Add a quick tour of the project to CONTRIBUTING (#30187) Add stricter geohash parsing (#30376) Reindex: Use request flavored methods (#30317) Silence SplitIndexIT.testSplitIndexPrimaryTerm test failure. (#30432) Auto-expand replicas when adding or removing nodes (#30423) Silence IndexUpgradeIT test failures. (#30430) Fix line length violation in cache tests Add failing test for core cache deadlock [DOCS] convert forcemerge snippet Update forcemerge.asciidoc (#30113) Added zentity to the list of API extension plugins (#29143) Fix the search request default operation behavior doc (#29302) (#29405) Watcher: Mark watcher as started only after loading watches (#30403) Correct wording in log message (#30336) Do not fail snapshot when deleting a missing snapshotted file (#30332) AwaitsFix testCreateShrinkIndexToN DOCS: Correct mapping tags in put-template api DOCS: Fix broken link in the put index template api Add put index template api to high level rest client (#30400) [Docs] Add snippets for POS stop tags default value Remove entry inadvertently picked into changelog Move respect accept header on no handler to 6.3.1 Respect accept header on no handler (#30383) [Test] Add analysis-nori plugin to the vagrant tests [Rollup] Validate timezone in range queries (#30338) [Docs] Fix bad link [Docs] Fix end of section in the korean plugin docs add the Korean nori plugin to the change logs Expose the Lucene Korean analyzer module in a plugin (#30397) Docs: remove transport_client from CCS role example (#30263) Test: remove cluster permission from CCS user (#30262) Watcher: Remove unneeded index deletion in tests fix docs branch version fix lucene snapshot version Upgrade to 7.4.0-snapshot-1ed95c097b (#30357) [ML][TEST] Clean up jobs in ModelPlotIT Watcher: Ensure trigger service pauses execution (#30363) [DOCS] Fixes ordering of changelog sections [DOCS] Commented out empty sections in the changelog to fix the doc build. (#30372) Make RepositoriesMetaData contents unmodifiable (#30361) Change signature of Get Repositories Response (#30333) 6.x Backport: Terms query validate bug (#30319) InternalEngineTests.testConcurrentOutOfOrderDocsOnReplica should use two documents (#30121) Security: reduce garbage during index resolution (#30180) Test: use trial license in qa tests with security [ML] Add integration test for model plots (#30359) SQL: Fix bug caused by empty composites (#30343) [ML] Account for gaps in data counts after job is reopened (#30294) [ML] Refactor DataStreamDiagnostics to use array (#30129) Make licensing FIPS-140 compliant (#30251) Do not load global state when deleting a snapshot (#29278) Don't load global state when only restoring indices (#29239) Tests: Use different watch ids per test in smoke test (#30331) Watcher: Make start/stop cycle more predictable and synchronous (#30118) [Docs] Add term query with normalizer example Adds Eclipse config for xpack licence headers (#30299) Fix message content in users tool (#30293) [DOCS] Removed X-Pack breaking changes page [DOCS] Added security breaking change [DOCS] Fixes link to TLS LDAP info [DOCS] Merges X-Pack release notes into changelog (#30350) [DOCS] Fixes broken links to bootstrap user (#30349) [Docs] Remove errant changelog line Fix NPE when CumulativeSum agg encounters null/empty bucket (#29641) [DOCS] Reorganizes authentication details in Stack Overview (#30280) Tests: Simplify VersionUtils released version splitting (#30322) Fix merging logic of Suggester Options (#29514) ReplicationTracker.markAllocationIdAsInSync may hang if allocation is cancelled (#30316) [DOCS] Adds LDAP realm configuration details (#30214) [DOCS] Adds native realm configuration details (#30215) Disable SSL on testing old BWC nodes (#30337) [DOCS] Enables edit links for X-Pack pages Cancelling a peer recovery on the source can leak a primary permit (#30318) SQL: Reduce number of ranges generated for comparisons (#30267) [DOCS] Adds links to changelog sections Convert server javadoc to html5 (#30279) REST Client: Add Request object flavored methods (#29623) Create default ES_TMPDIR on Windows (#30325) [Docs] Clarify `fuzzy_like_this` redirect (#30183) Fix docs of the `_ignored` meta field. Add a new `_ignored` meta field. (#29658) Move repository-azure fixture test to QA project (#30253)

…or-you * elastic/master: (22 commits) Docs: Test examples that recreate lang analyzers (elastic#29535) BulkProcessor to retry based on status code (elastic#29329) Add GET Repository High Level REST API (elastic#30362) add a comment explaining the need for RetryOnReplicaException on missing mappings Add `coordinating_only` node selector (elastic#30313) Stop forking groovyc (elastic#30471) Avoid setting connection request timeout (elastic#30384) Use date format in `date_range` mapping before fallback to default (elastic#29310) Watcher: Increase HttpClient parallel sent requests (elastic#30130) Mute ML upgrade test (elastic#30458) Stop forking javac (elastic#30462) Client: Deprecate many argument performRequest (elastic#30315) Docs: Use task_id in examples of tasks (elastic#30436) Security: Rename IndexLifecycleManager to SecurityIndexManager (elastic#30442) [Docs] Fix typo in cardinality-aggregation.asciidoc (elastic#30434) Avoid NPE in `more_like_this` when field has zero tokens (elastic#30365) Build: Switch to building javadoc with html5 (elastic#30440) Add a quick tour of the project to CONTRIBUTING (elastic#30187) Reindex: Use request flavored methods (elastic#30317) Silence SplitIndexIT.testSplitIndexPrimaryTerm test failure. (elastic#30432) ...

nik9000 added >non-issue review :Core/Infra/Core Core issues without another label v7.0.0 v6.3.0 v6.4.0 labels Apr 26, 2018

cbuescher requested changes Apr 27, 2018

View reviewed changes

Reorganize

f32f950

cbuescher requested changes Apr 28, 2018

View reviewed changes

nik9000 added 3 commits April 30, 2018 11:39

Cleanup

c5fcc61

:

baad66d

less dots

3de0f14

cbuescher reviewed May 2, 2018

View reviewed changes

cbuescher approved these changes May 2, 2018

View reviewed changes

Sneaky `

6e8a4de

nik9000 merged commit 6884b79 into elastic:master May 7, 2018

jimczi added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Add a quick tour of the project to CONTRIBUTING #30187

Add a quick tour of the project to CONTRIBUTING #30187

Conversation

nik9000 commented Apr 26, 2018

elasticmachine commented Apr 26, 2018

nik9000 commented Apr 27, 2018

cbuescher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbuescher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nik9000 commented Apr 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbuescher left a comment

Choose a reason for hiding this comment

nik9000 commented May 7, 2018