Page MenuHome

Algolia → ElasticSearch
AbandonedPublic

Authored by Stephan preeker (Preeker) on Dec 8 2017, 5:08 PM.

Diff Detail

Repository
rPS Pillar
Branch
elastic
Build Status
Buildable 1062
Build 1062: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
pillar/celery/search_index_tasks.py
28

this should be logged at debug level, if at all.

83

use an explicit class instead of a dict; see see comments about prepare_user_data(); they apply here too.

99

just use if node.get('description'):

137

remove newline

143

remove newline

144

this is not true -- the user hasn't been pushed yet at this point in time.

160

This isn't called by magic "when a user item is updated".

Also, the function name should reflect what the function does, not necessarily when it should be called. This is also inconsistent with node_save(), which is apparently named with a different naming strategy.

185

Algolia is used even when SEARCH_BACKENDS does not include it.

pillar/cli/elastic.py
13

Just name it manager_elastic to be consistent with other manager_xxx living in module xxx.

14

Usage information shouldn't include function calls.

27

Be more explicit in what parameters the function accepts, so that it can be understood from its --help message (rather than from reading the source code).

48

Log *what* is indexed; "%d users" is more descriptive than "%d".

66

Either refactor into common code, or just leave as-is but remove the "stolen from..." comment.

70

This is not done; stealing is accepable, but be careful not to copy too much.

80

pipeline is unused but _public_project_ids() is called twice.

85

Just use in-line dict here, no need to use a separate variable.

85

Here you fetch all nodes in public projects, but later in prepare_node_data() you also discard non-published nodes. Just filter them here too, so that you don't have to do any processing on them.

88

see comment about users

97

except is not a function call; space after it.

Also, what exactly would cause a KeyError or AttributeError? Why choose to handle those here, and not in prepare_node_data or index_node_save?

113

Be explicit when an unknown index name is given. Make sure that --help shows which index names can be used.

pillar/config.py
81–109

This should be a set and not a list, since order doesn't matter and duplicates are not allowed.

83

Add comments to each configuration option to explain what it does.

src/scripts/algolia_search.js
11

Delete unused code.

13

what is too ambiguous. "What I'm looking for" (e.g. the goal of the search) is something radically different than "what my search query is" (e.g. my attempt at reaching that goal), and maybe this even means something else.

23

Remove unnecessary variables. Just use elasticSearcher. Also, add a comment that explains where this variable comes from.

25

remove unused code.

33

See earlier note: what you are looking for is not the same as the search query (one is well-defined, the other is not).

34

This immediately overwrites the default value, so just set the default to this.

40

avoid unnecessary braces.

46

Clean up indentation & code.

49

Space between ){

140

facets is not defined at an outer scope, so assigning it here is moot.

144

This code *really* needs documentation about what it is doing and why it is doing it that way. And some cleanup. Without that I can't review it (as I don't know what your intent is, I can only guess, and that won't make for a good review).

149

"refined" is a concept that should be documented, or at least a comment that explains what it is should be included.

215

Just calculate the proper bounds for the for-loop, instead of looping over more than you need.

224

don't keep commented-out code

234

Why this strange off-by-one? I would expect prev_page = content.page - 1 and next_page = content.page + 1 (of course with some boundary checks).

263

keep indentations clean

268

delete unused code

358

No need to concatenate, just set var urlParams = '?q=' + … here.

359

Here again there is a strange base-1 to base-0 translation. If different parts of the same page use different bases for the pagination, this should be very explicit.

362

Remove unused code

376

remove newline

src/scripts/tutti/4_search.js
3

Remove algolia reference

146

First opening parenthesis is not necessary (and neither is the matching closing one). Also remove those from the other function declarations, unless they are really necessary (but then add a comment explaining why).

147–148

don't declare this as a JQuery function, because you never use $('some-selector').getSearch(). As far as I can see, this can just be implemented as part of findMatches

147–148

unused code

148

What is deze? This function follows a very specific design, which should be explained a bit in a comment.

156

Handle error conditions!

162

Document why it is necessary to call slice(0) twice.

179

Document which types are supported, and what the callback function will get as parameters.

183

Be clear in your documentation

184

Document why the unused data parameter is there, or remove it.

188

Document what acceptable values are.

196

Document what "being refined" means.

203

Write full English sentences, space after //, remove double spaces.

215

Use .done() and .fail() callbacks. Handle error conditions!

222

remove unused code

251

Remove console.log() calls unless end users have a real use for them.

252

Document what q, cb, and async are. I'm unsure what async is for, as it's called equally synchronously as cb.

src/scripts/tutti/7_user_search.js
8

fix indentation

src/templates/users/index.pug
25

Document what this URL-like thing is.

Sybren A. Stüvel (sybren) requested changes to this revision.Dec 13 2017, 4:39 PM
This revision now requires changes to proceed.Dec 13 2017, 4:39 PM

According to 533544117bf3 searching on user ID should work, but it doesn't in /u/ (even after reindexing users).

Stephan preeker (Preeker) marked 41 inline comments as done.Dec 15 2017, 5:52 PM
Stephan preeker (Preeker) added inline comments.
pillar/api/search/documents.py
29

this is a Aesthetics point of view. When developing testing each field/parameter might need to be changed / commented out. configuring/edits and changes are way easier this way.

120

its called defensive programmming :)

148

I have no clue. logic was there in algolia as well..

pillar/api/search/elastic_indexing.py
30

if prefer it there?

pillar/api/search/queries.py
81

unfortinatly using the logger, we lose all white space in the logger makeing query json debugging impossible...so i resorted to a print :)

pillar/api/search/routes.py
71

this should be checked...

pillar/celery/search_index_tasks.py
83

this was "inherited" old code :) it is a nested structure with no methods. I dont think a class thing would help.

pillar/cli/elastic.py
97

data integrity stuff. objects missing expected values.. this especially happend before I applied filtering..We do want to keep indexing and not completely come to an halt. In all other occasions it should crash. so I handle the execeptions here.

113

couldnt find an exmple to do this..

pillar/celery/search_index_tasks.py
83

A class would definitely help, because then you can go to its definition and see its fields, declare variables/parameters to be instances of that class, check with isinstance() if you want to be sure at runtime, etc.

This is especially important when passing from function to function, and even more so when different things are all called "user" or "nodes"

pillar/cli/elastic.py
97

Fair enough; something this would be great to put in the code as a comment.

113

Then find a solution; an alternative would be to have three separate reindex_{all,users,nodes} functions.

Stephan preeker (Preeker) marked 3 inline comments as done.Dec 29 2017, 2:57 PM
Stephan preeker (Preeker) updated this revision to Diff 9740.
  • wip D2950
  • wip D2950
  • Merge branch 'master' of git.blender.org:pillar into elastic
  • remove algolia from css and vendor stuff
  • javascript debugging
  • remove dead code
  • make javascript more secure..
Stephan preeker (Preeker) marked 2 inline comments as done.Dec 29 2017, 5:05 PM
pillar/api/search/index.py
32

Why doesn't this use the connection created in elastic_indexing.py?

Sybren A. Stüvel (sybren) requested changes to this revision.Jan 3 2018, 6:21 PM
Sybren A. Stüvel (sybren) added inline comments.
pillar/api/search/index.py
48

This should not log the same as a NotFoundError, as it's executed when there is *not* an exception.

This revision now requires changes to proceed.Jan 3 2018, 6:21 PM
  • better error reporting
  • improve elastic server settings
  • Merge branch 'master' of git.blender.org:pillar into elastic
  • merge
Stephan preeker (Preeker) marked 28 inline comments as done.Jan 5 2018, 11:57 AM
  • minor documentation / annotation fixes

Abandoning this review because we've already moved to ElasticSearch quite a while ago :)