Support cluster purge #7

chguocloudant · 2016-11-30T10:45:16Z

add 2 APIs(set_purge_seq, get_purge_seq).
add a test case to test the new APIs.

rnewson · 2016-11-30T16:59:03Z

where is purgeSeq persisted to the index?

mayya-sharipova · 2016-11-30T18:09:07Z

src/main/scala/com/cloudant/clouseau/IndexService.scala

@@ -70,6 +70,7 @@ class IndexService(ctx: ServiceContext[IndexServiceArgs]) extends Service(ctx) w
 var reader = DirectoryReader.open(ctx.args.writer, true)
 var updateSeq = getCommittedSeq
 var pendingSeq = updateSeq
+ var purgeSeq = 0L


why the default value is 0? Similarly to updateSeq it should be read from commitData on disk

rnewson · 2016-12-20T10:44:41Z

src/main/scala/com/cloudant/clouseau/IndexService.scala

@@ -199,6 +206,26 @@ class IndexService(ctx: ServiceContext[IndexServiceArgs]) extends Service(ctx) w
 }
 }

+ private def commitPurgeSeq(newSeq: Long) {


This method does not set the committing flag and so could allow concurrent attempts to commit, leading to a broken index (one with the wrong values).

This work should instead be done in the preceding commit function.

You mean we can commit purgeSeq and updateSeq with commit function?
We can change commit function with 2 arguments to support it.

private def commit(newSeq: Long, type: String) { if (type == "updateSeq") { ... } else { ... }

- add 2 APIs(set_purge_seq, get_purge_seq). - keep purge_seq in clouseau. - adjust the commit logic to update purge seq. - update the test code. BugzID: 68280

rnewson

Approved, nicely done. Please don't merge until we have the matching dreyfus work at the same level in case it changes what we need here.

rnewson · 2016-12-22T14:24:37Z

src/main/scala/com/cloudant/clouseau/IndexService.scala

@@ -569,7 +580,8 @@ class IndexService(ctx: ServiceContext[IndexServiceArgs]) extends Service(ctx) w
 ('doc_count, reader.numDocs),
 ('doc_del_count, reader.numDeletedDocs),
 ('pending_seq, pendingSeq),
- ('committed_seq, getCommittedSeq)
+ ('committed_seq, getCommittedSeq),
+ ('purge_seq, pendingPurgeSeq)


should be purgeSeq here not pendingPurgeSeq.

OK, I will change it later.

chguocloudant · 2016-12-22T15:24:18Z

src/main/scala/com/cloudant/clouseau/IndexService.scala

+ case SetPurgeSeqMsg(newSeq: Long) =>
+ pendingPurgeSeq = newSeq
+ logger.debug("Pending purge sequence is now %d".format(newSeq))
+ commit(pendingSeq, pendingPurgeSeq)


Hi @rnewson .
I triggered commit when the purgeSeq is updated. Will it cost too much?

And I think the var pendingPurgeSeq can be removed if the commit is allowed. Right?

hm, good question. I'd rather we always commit asynchronously and on the established interval. For users that purge a lot (and there will be a lot of them if this work is extended to allow 'hard' deletes) they'll be committing very frequently and this will hurt performance.

This means the purge seq message could be processed but not actioned, when clouseau crashes before committing, so we'll need to ensure that dreyfus will retry (it should, since it reads the purge seq from the index, but let's be sure).

Hi @rnewson .
If we commit purgeSeq asynchronously and also the users purge a lot, the dreyfus will retry to delete the index again in clouseau.
For example, if dreyfus gets a purgeSeq(6) from couchdb and gets an IdxPurgeSeq(5) from clouseau, it will purge index from 5 and set purgeSeq to 6.
If clouseau is crashed before committing, dreyfus will get purgeSeq(5) again. And dreyfus will purge index from 5 again.
But I am not sure which is more expensive, delete index or commit?

we can safely assume clouseau crashes are rare from our production experience.

commit is expensive, the most expensive operation there is in Lucene, I think.

So the choice is between an expensive commit for every purge or a less expensive delete on retry for every clouseau crash.

We should also not say "delete index" when we really mean "delete document". :)

OK. I think I can modify it later.
Let me make sure that we can remove the value pendingPurgeSeq, right? If the dreyfus calls get_purge_seq, clouseau will return purgeSeq directly(now this value maybe is not committed yet).

I don't see how we can remove pending. we need to return the committed purge seq when asked and need to retain the pending value for when we commit.

- adjust the commit logic to update purge seq. BugzID: 68280

rnewson · 2017-01-06T16:05:47Z

src/main/scala/com/cloudant/clouseau/IndexService.scala

@@ -117,6 +120,10 @@ class IndexService(ctx: ServiceContext[IndexServiceArgs]) extends Service(ctx) w
 pendingSeq = newSeq
 logger.debug("Pending sequence is now %d".format(newSeq))
 'ok
+ case SetPurgeSeqMsg(newPurgeSeq: Long) =>
+ purgeSeq = newPurgeSeq


this should be pendingPurgeSeq = newPurgeSeq. purgeSeq must only change after it's been committed.

Got it. I will modify it.

- use pendingPurgeSeq in set_purge_seq and get_purge_seq. - update purgeSeq when pengdingPurgeSeq is committed. BugzId: 68280

rnewson

+1 but let's merge once the matching dreyfus PR is ready.

rnewson · 2017-01-09T19:06:00Z

src/main/scala/com/cloudant/clouseau/IndexService.scala

@@ -117,6 +121,10 @@ class IndexService(ctx: ServiceContext[IndexServiceArgs]) extends Service(ctx) w
 pendingSeq = newSeq
 logger.debug("Pending sequence is now %d".format(newSeq))
 'ok
+ case SetPurgeSeqMsg(newPurgeSeq: Long) =>
+ pendingPurgeSeq = newPurgeSeq
+ logger.debug("purge sequence is now %d".format(newPurgeSeq))


should say 'Pending purge sequence is now %d'

OK, got it. I will modify it later.

mayya-sharipova reviewed Nov 30, 2016

View reviewed changes

rnewson changed the title ~~Support clouster purge~~ Support cluster purge Nov 30, 2016

rnewson reviewed Dec 20, 2016

View reviewed changes

Support clustered purge

a8c3c8e

- add 2 APIs(set_purge_seq, get_purge_seq). - keep purge_seq in clouseau. - adjust the commit logic to update purge seq. - update the test code. BugzID: 68280

chguocloudant force-pushed the 68280_clouseau_for_clustered_purge branch from 86f9452 to a8c3c8e Compare December 22, 2016 02:27

rnewson approved these changes Dec 22, 2016

View reviewed changes

chguocloudant commented Dec 22, 2016

View reviewed changes

Support clustered purge

8c462eb

- adjust the commit logic to update purge seq. BugzID: 68280

rnewson requested changes Jan 6, 2017

View reviewed changes

Update for set/get purge seq

6bd12a2

- use pendingPurgeSeq in set_purge_seq and get_purge_seq. - update purgeSeq when pengdingPurgeSeq is committed. BugzId: 68280

rnewson approved these changes Jan 9, 2017

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support cluster purge #7

Support cluster purge #7

chguocloudant commented Nov 30, 2016

rnewson commented Nov 30, 2016 •

edited

Loading

mayya-sharipova Nov 30, 2016 •

edited

Loading

rnewson Dec 20, 2016

chguocloudant Dec 21, 2016

rnewson left a comment

rnewson Dec 22, 2016

chguocloudant Dec 22, 2016

chguocloudant Dec 22, 2016

chguocloudant Dec 23, 2016

rnewson Jan 3, 2017

chguocloudant Jan 3, 2017

rnewson Jan 3, 2017

chguocloudant Jan 3, 2017

rnewson Jan 3, 2017

rnewson Jan 6, 2017

chguocloudant Jan 9, 2017

rnewson left a comment

rnewson Jan 9, 2017

chguocloudant Jan 10, 2017

Support cluster purge #7

Are you sure you want to change the base?

Support cluster purge #7

Conversation

chguocloudant commented Nov 30, 2016

rnewson commented Nov 30, 2016 • edited Loading

mayya-sharipova Nov 30, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rnewson 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

rnewson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rnewson commented Nov 30, 2016 •

edited

Loading

mayya-sharipova Nov 30, 2016 •

edited

Loading