[WIP] feat: delete unconfirmed users #414

pajgo · 2019-07-30T12:35:17Z

Please comment.

codecov · 2019-07-31T13:32:40Z

Codecov Report

Merging #414 into master will decrease coverage by 0.08%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #414      +/-   ##
==========================================
- Coverage   94.57%   94.48%   -0.09%     
==========================================
  Files         130      130              
  Lines        2341     2341              
==========================================
- Hits         2214     2212       -2     
- Misses        127      129       +2

Impacted Files	Coverage Δ
src/users.js	`88.33% <0%> (-3.34%)`	⬇️

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 0c2d22b...38daae5. Read the comment docs.

codecov · 2019-07-31T13:32:41Z

Codecov Report

Merging #414 into master will increase coverage by 0.06%.
The diff coverage is 99.11%.

@@            Coverage Diff             @@
##           master     #414      +/-   ##
==========================================
+ Coverage   94.53%   94.59%   +0.06%     
==========================================
  Files         130      132       +2     
  Lines        2377     2422      +45     
==========================================
+ Hits         2247     2291      +44     
- Misses        130      131       +1

Impacted Files	Coverage Δ
src/constants.js	`100% <ø> (ø)`	⬆️
src/utils/inactiveUsers/index.js	`100% <100%> (ø)`
src/utils/updateMetadata.js	`100% <100%> (ø)`	⬆️
src/utils/inactiveUsers/delete.js	`100% <100%> (ø)`
src/actions/remove.js	`95.83% <100%> (+0.18%)`	⬆️
src/actions/register.js	`96.37% <100%> (+0.05%)`	⬆️
src/users.js	`92.18% <100%> (+0.52%)`	⬆️
src/actions/activate.js	`85.24% <100%> (+0.5%)`	⬆️
src/utils/setOrganizationMetadata.js	`90.9% <100%> (+0.9%)`	⬆️
src/utils/inactiveUsers/defineCommand.js	`96.77% <96.77%> (ø)`
... and 4 more

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 f6e1f97...9b773a5. Read the comment docs.

AVVS · 2019-07-31T19:59:27Z

doc/rfcs/inactive_users_cleanup.md

+
+### addInactiveUser(id, audience)
+Adds corresponding id record into `USERS_ACTIVATE` with score eq current time. Sets `USERS_ACTIVATE_AUDIENCE` hash to track audieces provided in registration process
+


User may have multiple audiences associated with it and right now its one of the problems during removal of such users. We may need to add tracking of used audiences as well when updateMetadata methods are used so that we have references to them and cleanup the data in them as well

0dc98f2 audience tracking and cleanup already in commit. Possible solution, describes pre logic. Current logic in pre text

AVVS · 2019-07-31T20:01:22Z

doc/rfcs/inactive_users_cleanup.md

+- Create additional set with id's of inactive users (`users-activate`).
+  * Change activation process: remove successfully activated user id from `users-activate`.
+  * Change registration process: insert inactive user id into `users-activate`.
+- Add repeating task with interval `config.incativeCleanupInterval`.


I'd like to explore another venue where we perform cleanup for all inactive users during each registration and/or activation - process needs to very quick and written in LUA so that to be atomic - then we dont need to scan database and sets of pointers will be quick to use

AVVS · 2019-07-31T20:03:26Z

src/utils/inactiveUsers.js

+  const lock = await this.dlock.once(lockKey);
+  const inactiveAccounts = await getInactiveUsers(redis, deleteInactiveAccounts);
+
+  inactiveAccounts.forEach(async (acc) => {


async/await doesnt work that way - this will launch a bunch of unhandled promises - if you need to actually await for top level promises - the way to do it is to have await on Promise.all calls

const jobs = inactiveAccounts.map(...)
await Promise.all(jobs)

But I'd just suggest using Promise.map/mapSeries util from bluebird for that case

AVVS · 2019-07-31T20:04:18Z

src/utils/inactiveUsers.js

+ * @returns {NodeJS.Timeout}
+ */
+function getCleanerTask(service) {
+  const { deleteInactiveAccountsInterval: interval } = service.config;


multiple instances would conflict - we want to scale this up easily and, even though, lock would generally prevent things from breaking - I'd suggest to avoid them if possible

AVVS · 2019-08-06T14:57:52Z

src/actions/activate.js

@@ -192,6 +195,8 @@ function activateAction({ params }) {
  };

  return Promise
+    .resolve(['users:cleanup'])
+    .spread(this.hook.bind(this))


Promise.bind(this,['users:cleanup']
.spread(this.hook)

AVVS · 2019-08-06T14:58:40Z

src/actions/register.js

@@ -348,7 +349,11 @@ module.exports = async function registerUser({ params }) {
    .disposer(lockDisposer);

  return Promise
-    .using({ service, params }, acquireLock, performRegistration)
+    .resolve(['users:cleanup'])
+    .spread(service.hook.bind(service))


same thing with bind & use async/await when possible

AVVS · 2019-08-07T06:31:29Z

scripts/deleteInactivated.lua

@@ -148,6 +148,7 @@ local function cleanInactiveUsers(idList)
 end

 -- Command body
+redis.replicate_commands();


makes sense to use that in other scripts?

makes sense to use that in other scripts?

A bit, it stabilizes script behavior for older Redis versions and forces them to use 5.0 version behavior. IMHO Lua evaluation and processing on replica nodes bring additional overhead.

AVVS · 2019-08-12T19:33:54Z

src/actions/activate.js

@@ -126,6 +127,8 @@ function activateAccount(data, metadata) {
    .persist(userKey)
    .sadd(USERS_INDEX, userId);

+  removeInactiveUser(pipeline, userId);


не очень понятное название переменной, removeFromInactiveUsers будет понятнее
а то кажется что юзера удаляем вообще

AVVS · 2019-08-12T19:34:06Z

src/actions/register.js

@@ -208,7 +209,7 @@ async function performRegistration({ service, params }) {
  pipeline.hset(USERS_USERNAME_TO_ID, username, userId);

  if (activate === false && config.deleteInactiveAccounts >= 0) {
-    pipeline.expire(userDataKey, config.deleteInactiveAccounts);
+    addInactiveUser(pipeline, userId, audience);


тут тоже лучше addToInactiveUsers

AVVS · 2019-08-12T19:35:31Z

src/actions/register.js

@@ -348,7 +349,11 @@ module.exports = async function registerUser({ params }) {
    .disposer(lockDisposer);

  return Promise
-    .using({ service, params }, acquireLock, performRegistration)
+    .bind(this, ['users:cleanup'])


клинап не стоит проводить каждый раз когда вызывается регистрация, лучше непосредственно внутри цикла когда взяли лок, операция потенциально дорогая, лучше делать до проверки есть ли такой юзер уже

AVVS · 2019-08-12T19:38:29Z

src/utils/updateMetadata.js

+}
+
+async function setAudience(redis, userId, audience) {
+  return redis.hset(USERS_AUDIENCE, userId, JSONStringify(audience));


если 2 параллельных запроса будут обновлять audience, то одно может перезаписать другое
такие обновления нужно делать в lua (где нужно get+set) и как сайд эффект обновления данных внутри

соответственно это предполагает что метод updateMetadata будет обновлен и то где мы добавляем/удаляем поля будет происходить через lua, в конце выполнения луа мы будем проверять сколько полей по обновляемому audience у хэша, и если полей 0 - удаляем из сета audience по конкретному юзеру, а если > 0 , то добавляем

bludzhd · 2019-08-23T16:37:19Z

rfcs/inactive_users_cleanup.md

+When Activation action executes, the hook starts before all actions. This strategy saves from inactive users 
+that hit TTL but tried to pass the activation process.
+When Registration action executed, hook executed after `username` exists check.
+Handler not breaking general logic and not throwing errors, logs error into a logfile.


Hook handler doesn't breake general logic or throw errors, just logs it. (🤔 ?)

bludzhd · 2019-08-23T16:45:04Z

rfcs/inactive_users_cleanup.md

+
+## Registration and activation
+On Activation and Registration request event `users:cleanup` is emitted, and executed as a hook.
+When Activation action executes, the hook starts before all actions. This strategy saves from inactive users 


When Activation action is called, the hook is invoked/fired/called first.

AVVS · 2019-08-25T18:12:05Z

src/utils/inactiveUsers/index.js

+  try {
+    deletedUsers = await deleteInactiveUsers(redis, deleteInactiveAccounts);
+  } catch (e) {
+    this.log.error(e);


никогда не будет выбрасывать ошибку, это проблема - если нам нужно сапрессить ошибку, то следует это давать делать в функции вне этой

более того, если мы используем логгер, то корректно делать так:

log.error({ err: e }, message) иначе ошибка не будет нормально десериализована

AVVS · 2019-08-25T18:13:14Z

src/utils/updateMetadata.js

+// Stabilizes Lua script response
+function mapUpdateResponse(jsonStr) {
+  const decodedData = JSONParse(jsonStr);
+  const result = ld.map(decodedData, (audienceProcessResult) => {


можем ли мы использовать for ... of Object.enries(..) ?

AVVS · 2019-08-27T17:28:42Z

src/utils/inactiveUsers/index.js

+      );
+    }
+    return [...prev, ...delTokenActions];
+  }, []);


- const work = userIds.reduce((prev, id) => { - const delTokenActions = []; - for (const action of actions) { - delTokenActions.push( - this.tokenManager.remove({ id, action }) - ); - } - return [...prev, ...delTokenActions]; - }, []); + const work = userIds.reduce((accum, id) => { + for (const action of actions) { + accum.push(this.tokenManager.remove({ id, action })); + } + return accum; + }, []);

* rfcs

* code

* tests

* doc lex and typo fixes

* cluster bug fix

* updateMetadata no lodash * doc addons * cleanUsers supperess error parameter

* organization action tokens remove * user action tokens remove

* fs.promises use * delete token array reassignment fix * doc update

* linter parantheses fix

pajgo · 2019-11-08T11:30:14Z

Closed as duplicate #430

pajgo force-pushed the feat/deleteUnconfirmedUsers branch from e4dc806 to 38daae5 Compare July 31, 2019 13:32

pajgo force-pushed the feat/deleteUnconfirmedUsers branch from 38daae5 to 0dc98f2 Compare July 31, 2019 13:55

AVVS reviewed Jul 31, 2019

View reviewed changes

pajgo force-pushed the feat/deleteUnconfirmedUsers branch from 0dc98f2 to 6d5c429 Compare August 6, 2019 10:27

AVVS reviewed Aug 6, 2019

View reviewed changes

AVVS reviewed Aug 7, 2019

View reviewed changes

AVVS reviewed Aug 12, 2019

View reviewed changes

pajgo force-pushed the feat/deleteUnconfirmedUsers branch from 402c67b to acf50bb Compare August 15, 2019 07:30

pajgo marked this pull request as ready for review August 20, 2019 14:04

pajgo force-pushed the feat/deleteUnconfirmedUsers branch 2 times, most recently from b43a1f4 to e1c48a0 Compare August 23, 2019 12:40

bludzhd reviewed Aug 23, 2019

View reviewed changes

AVVS reviewed Aug 25, 2019

View reviewed changes

AVVS reviewed Aug 27, 2019

View reviewed changes

pajgo added 5 commits September 15, 2019 19:23

feat: delete inactivated users

ab7664c

* rfcs

feat: delete inactivated users

3d66715

* code

feat: delete inactivated users

c42e14a

* tests

feat: delete inactivated users

77ce48b

* doc lex and typo fixes

feat: delete inactivated users

879ebb5

* cluster bug fix

pajgo added 4 commits September 15, 2019 19:26

feat: delete inactivated users

9309387

* updateMetadata no lodash * doc addons * cleanUsers supperess error parameter

feat: delete inactivated users

0ad48fa

* organization action tokens remove * user action tokens remove

feat: delete inactivated users

9557c32

* fs.promises use * delete token array reassignment fix * doc update

feat: delete inactivated users

9b773a5

* linter parantheses fix

pajgo force-pushed the feat/deleteUnconfirmedUsers branch from 9231318 to 9b773a5 Compare September 15, 2019 14:03

pajgo changed the title ~~feat: delete unconfirmed users~~ [WIP] feat: delete unconfirmed users Sep 26, 2019

pajgo changed the base branch from master to pajgo-feat/delete-inactive-users September 27, 2019 12:29

pajgo force-pushed the pajgo-feat/delete-inactive-users branch from 25186e7 to d9f0aad Compare October 21, 2019 15:02

pajgo closed this Nov 8, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] feat: delete unconfirmed users #414

[WIP] feat: delete unconfirmed users #414

pajgo commented Jul 30, 2019

codecov bot commented Jul 31, 2019

codecov bot commented Jul 31, 2019 •

edited

Loading

AVVS Jul 31, 2019

pajgo Jul 31, 2019

AVVS Jul 31, 2019

AVVS Jul 31, 2019

AVVS Jul 31, 2019

AVVS Aug 6, 2019

AVVS Aug 6, 2019

AVVS Aug 7, 2019

pajgo Aug 7, 2019 •

edited

Loading

AVVS Aug 12, 2019

AVVS Aug 12, 2019 •

edited

Loading

AVVS Aug 12, 2019

AVVS Aug 12, 2019

bludzhd Aug 23, 2019

bludzhd Aug 23, 2019

AVVS Aug 25, 2019

AVVS Aug 25, 2019

AVVS Aug 27, 2019

pajgo commented Nov 8, 2019


		### addInactiveUser(id, audience)
		Adds corresponding id record into `USERS_ACTIVATE` with score eq current time. Sets `USERS_ACTIVATE_AUDIENCE` hash to track audieces provided in registration process

[WIP] feat: delete unconfirmed users #414

[WIP] feat: delete unconfirmed users #414

Conversation

pajgo commented Jul 30, 2019

codecov bot commented Jul 31, 2019

Codecov Report

codecov bot commented Jul 31, 2019 • edited Loading

Codecov Report

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

pajgo Aug 7, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AVVS Aug 12, 2019 • edited Loading

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

pajgo commented Nov 8, 2019

codecov bot commented Jul 31, 2019 •

edited

Loading

pajgo Aug 7, 2019 •

edited

Loading

AVVS Aug 12, 2019 •

edited

Loading