1126153 - [meta] Deploy log-based replication for hg.mozilla.org

Status: RESOLVED FIXED

(Developer Services :: Mercurial: hg.mozilla.org, defect)

Description

[Gregory Szorc [:gps]]

One of my Q1 deliverables is to "fix" hg.mozilla.org replication. This entails some subset of:

* removing synchronous-with-push replication (without sacrificing durability)
* replicate pushkey changes at push time (bookmarks, phases, etc)
* better recovery for when machines go down and get out of sync
* foster migration of hg.mozilla.org out of SCL3 and into "the cloud" (almost certainly AWS)
* facilitate multiple data center hosting of hg.mozilla.org (this includes the ability for offices to have their own mirrors for super fast access)

Unless things change, the plan is to leverage Apache Kafka to host a log/queue of repository change events which will then be fanned out to all the replicas to initiate replication. This is conceptually similar to what we're doing now, except we're swapping the brittle, synchronous SSH signalling with a durable and asynchronous queue.

Comment 1

[Gregory Szorc [:gps]]

Attachment: MozReview Request: bz://1126153/gps

/r/3019 - testing: support for running a Kafka cluster
/r/3021 - testing: create topic as part of Kafka start
/r/3023 - testing: add kafka-python to virtualenv
/r/3025 - vcsreplicator: create project (bug 1126153)
/r/3027 - vcsreplicator: send a message when a repository is created
/r/3029 - vcsreplicator: implement basic consuming logic

Pull down these commits:

hg pull review -r d5f384247c7b19c965a41f991cefc5b4a12c0cb6

Comment 2

[Monty the Python Review Bot]

https://reviewboard.mozilla.org/r/3019/#review2329

Always look on the bright side of life.

I analyzed your Python changes and found 11 errors.

The following files were examined:

  testing/docker/builder-kafkabroker/entrypoint.py
  testing/docker/builder-kafkabroker/set-servers.py
  testing/vcttesting/docker.py
  testing/vcttesting/docker_mach_commands.py

::: testing/docker/builder-kafkabroker/entrypoint.py
(Diff revision 1)
> +   def __init__(self, s):

E111: indentation is not a multiple of four
Use 4 spaces per indentation level.

For really old code that you don't want to mess up, you can continue to
use 8-space tabs.

::: testing/docker/builder-kafkabroker/entrypoint.py
(Diff revision 1)
> +       self.s = s

E111: indentation is not a multiple of four
Use 4 spaces per indentation level.

For really old code that you don't want to mess up, you can continue to
use 8-space tabs.

::: testing/docker/builder-kafkabroker/entrypoint.py
(Diff revision 1)
> +   def write(self, d):

E111: indentation is not a multiple of four
Use 4 spaces per indentation level.

For really old code that you don't want to mess up, you can continue to
use 8-space tabs.

::: testing/docker/builder-kafkabroker/entrypoint.py
(Diff revision 1)
> +       self.s = s
> +   def write(self, d):

E301: expected 1 blank line, found 0
Separate top-level function and class definitions with two blank lines.

Method definitions inside a class are separated by a single blank line.

Extra blank lines may be used (sparingly) to separate groups of related
functions.  Blank lines may be omitted between a bunch of related
one-liners (e.g. a set of dummy implementations).

Use blank lines in functions, sparingly, to indicate logical sections.

::: testing/docker/builder-kafkabroker/entrypoint.py
(Diff revision 1)
> +       self.s.write(d)

E111: indentation is not a multiple of four
Use 4 spaces per indentation level.

For really old code that you don't want to mess up, you can continue to
use 8-space tabs.

::: testing/docker/builder-kafkabroker/entrypoint.py
(Diff revision 1)
> +       self.s.flush()

E111: indentation is not a multiple of four
Use 4 spaces per indentation level.

For really old code that you don't want to mess up, you can continue to
use 8-space tabs.

::: testing/docker/builder-kafkabroker/entrypoint.py
(Diff revision 1)
> +   def __getattr__(self, a):

E111: indentation is not a multiple of four
Use 4 spaces per indentation level.

For really old code that you don't want to mess up, you can continue to
use 8-space tabs.

::: testing/docker/builder-kafkabroker/entrypoint.py
(Diff revision 1)
> +       self.s.flush()
> +   def __getattr__(self, a):

E301: expected 1 blank line, found 0
Separate top-level function and class definitions with two blank lines.

Method definitions inside a class are separated by a single blank line.

Extra blank lines may be used (sparingly) to separate groups of related
functions.  Blank lines may be omitted between a bunch of related
one-liners (e.g. a set of dummy implementations).

Use blank lines in functions, sparingly, to indicate logical sections.

::: testing/docker/builder-kafkabroker/entrypoint.py
(Diff revision 1)
> +       return getattr(self.s, a)

E111: indentation is not a multiple of four
Use 4 spaces per indentation level.

For really old code that you don't want to mess up, you can continue to
use 8-space tabs.

::: testing/vcttesting/docker.py
(Diff revision 1)
> +                port = state['NetworkSettings']['Ports']['9092/tcp'][0]['HostPort']

E501: line too long (83 > 79 characters)
Limit all lines to a maximum of 79 characters.

There are still many devices around that are limited to 80 character
lines; plus, limiting windows to 80 characters makes it possible to have
several windows side-by-side.  The default wrapping on such devices looks
ugly.  Therefore, please limit all lines to a maximum of 79 characters.
For flowing long blocks of text (docstrings or comments), limiting the
length to 72 characters is recommended.

Reports error E501.

::: testing/vcttesting/docker_mach_commands.py
(Diff revision 1)
> +    @CommandArgument(
> +            '--shell', action='store_true',

E126: continuation line over-indented for hanging indent
Continuation lines indentation.

Continuation lines should align wrapped elements either vertically
using Python's implicit line joining inside parentheses, brackets
and braces, or using a hanging indent.

When using a hanging indent these considerations should be applied:
- there should be no arguments on the first line, and
- further indentation should be used to clearly distinguish itself as a
continuation line.

Comment 3

[Monty the Python Review Bot]

https://reviewboard.mozilla.org/r/3021/#review2331

And now for something completely different.

Congratulations, there we no Python static analysis issues with this patch!

The following files were examined:

  testing/vcttesting/docker.py

Comment 4

[Monty the Python Review Bot]

https://reviewboard.mozilla.org/r/3025/#review2333

Always look on the bright side of life.

I analyzed your Python changes and found 3 errors.

The following files were examined:

  pylib/vcsreplicator/setup.py
  pylib/vcsreplicator/vcsreplicator/__init__.py
  pylib/vcsreplicator/vcsreplicator/config.py
  pylib/vcsreplicator/vcsreplicator/hgext.py
  pylib/vcsreplicator/vcsreplicator/producer.py

::: pylib/vcsreplicator/vcsreplicator/config.py
(Diff revision 1)
> +    @property

E303: too many blank lines (2)
Separate top-level function and class definitions with two blank lines.

Method definitions inside a class are separated by a single blank line.

Extra blank lines may be used (sparingly) to separate groups of related
functions.  Blank lines may be omitted between a bunch of related
one-liners (e.g. a set of dummy implementations).

Use blank lines in functions, sparingly, to indicate logical sections.

::: pylib/vcsreplicator/vcsreplicator/config.py
(Diff revision 1)
> +            self._producer = Producer(
> +                    client, topic, partition, batch_send=False,

E126: continuation line over-indented for hanging indent
Continuation lines indentation.

Continuation lines should align wrapped elements either vertically
using Python's implicit line joining inside parentheses, brackets
and braces, or using a hanging indent.

When using a hanging indent these considerations should be applied:
- there should be no arguments on the first line, and
- further indentation should be used to clearly distinguish itself as a
continuation line.

::: pylib/vcsreplicator/vcsreplicator/producer.py
(Diff revision 1)
> +        return super(Producer, self).send_messages(
> +                self.topic, self.partition, msg)

E126: continuation line over-indented for hanging indent
Continuation lines indentation.

Continuation lines should align wrapped elements either vertically
using Python's implicit line joining inside parentheses, brackets
and braces, or using a hanging indent.

When using a hanging indent these considerations should be applied:
- there should be no arguments on the first line, and
- further indentation should be used to clearly distinguish itself as a
continuation line.

Comment 5

[Monty the Python Review Bot]

https://reviewboard.mozilla.org/r/3027/#review2335

And now for something completely different.

Congratulations, there we no Python static analysis issues with this patch!

The following files were examined:

  pylib/vcsreplicator/vcsreplicator/hgext.py
  pylib/vcsreplicator/vcsreplicator/producer.py

Comment 6

[Monty the Python Review Bot]

https://reviewboard.mozilla.org/r/3029/#review2337

Always look on the bright side of life.

I analyzed your Python changes and found 1 errors.

The following files were examined:

  pylib/vcsreplicator/vcsreplicator/config.py
  pylib/vcsreplicator/vcsreplicator/consumer.py

::: pylib/vcsreplicator/vcsreplicator/consumer.py
(Diff revision 1)
> +        super(Consumer, self).__init__(
> +                client, group, topic, partitions=partitions,

E126: continuation line over-indented for hanging indent
Continuation lines indentation.

Continuation lines should align wrapped elements either vertically
using Python's implicit line joining inside parentheses, brackets
and braces, or using a hanging indent.

When using a hanging indent these considerations should be applied:
- there should be no arguments on the first line, and
- further indentation should be used to clearly distinguish itself as a
continuation line.

Comment 7

[Gregory Szorc [:gps]]

Comment on attachment 8555046 [details]
MozReview Request: bz://1126153/gps

/r/3019 - testing: support for running a Kafka cluster
/r/3021 - testing: create topic as part of Kafka start
/r/3023 - testing: add kafka-python to virtualenv
/r/3025 - vcsreplicator: create project (bug 1126153)
/r/3027 - vcsreplicator: send a message when a repository is created
/r/3029 - vcsreplicator: implement basic consuming logic
/r/3045 - vcsreplicator: make path normalization more useful
/r/3047 - testing: add python-hglib to requirements
/r/3049 - vcsreplicator: ability to disable event production
/r/3051 - vcsreplicator: create new repositories when init message is received
/r/3053 - vcsreplicator: factor common configuration into shared script
/r/3055 - vcsreplicator: add more helper methods to helpers.sh
/r/3057 - vcsreplicator: ability to record hgrc files

Pull down these commits:

hg pull review -r 1c96aace7545e689dd77856191c6ecc8fac64821

Comment 8

[Monty the Python Review Bot]

https://reviewboard.mozilla.org/r/3019/#review2351

Always look on the bright side of life.

I analyzed your Python changes and found 11 errors.

The following files were examined:

  testing/docker/builder-kafkabroker/entrypoint.py
  testing/docker/builder-kafkabroker/set-servers.py
  testing/vcttesting/docker.py
  testing/vcttesting/docker_mach_commands.py

::: testing/docker/builder-kafkabroker/entrypoint.py
(Diff revision 2)
> +   def __init__(self, s):

E111: indentation is not a multiple of four
Use 4 spaces per indentation level.

For really old code that you don't want to mess up, you can continue to
use 8-space tabs.

::: testing/docker/builder-kafkabroker/entrypoint.py
(Diff revision 2)
> +       self.s = s

E111: indentation is not a multiple of four
Use 4 spaces per indentation level.

For really old code that you don't want to mess up, you can continue to
use 8-space tabs.

::: testing/docker/builder-kafkabroker/entrypoint.py
(Diff revision 2)
> +   def write(self, d):

E111: indentation is not a multiple of four
Use 4 spaces per indentation level.

For really old code that you don't want to mess up, you can continue to
use 8-space tabs.

::: testing/docker/builder-kafkabroker/entrypoint.py
(Diff revision 2)
> +       self.s = s
> +   def write(self, d):

E301: expected 1 blank line, found 0
Separate top-level function and class definitions with two blank lines.

Method definitions inside a class are separated by a single blank line.

Extra blank lines may be used (sparingly) to separate groups of related
functions.  Blank lines may be omitted between a bunch of related
one-liners (e.g. a set of dummy implementations).

Use blank lines in functions, sparingly, to indicate logical sections.

::: testing/docker/builder-kafkabroker/entrypoint.py
(Diff revision 2)
> +       self.s.write(d)

E111: indentation is not a multiple of four
Use 4 spaces per indentation level.

For really old code that you don't want to mess up, you can continue to
use 8-space tabs.

::: testing/docker/builder-kafkabroker/entrypoint.py
(Diff revision 2)
> +       self.s.flush()

E111: indentation is not a multiple of four
Use 4 spaces per indentation level.

For really old code that you don't want to mess up, you can continue to
use 8-space tabs.

::: testing/docker/builder-kafkabroker/entrypoint.py
(Diff revision 2)
> +   def __getattr__(self, a):

E111: indentation is not a multiple of four
Use 4 spaces per indentation level.

For really old code that you don't want to mess up, you can continue to
use 8-space tabs.

::: testing/docker/builder-kafkabroker/entrypoint.py
(Diff revision 2)
> +       self.s.flush()
> +   def __getattr__(self, a):

E301: expected 1 blank line, found 0
Separate top-level function and class definitions with two blank lines.

Method definitions inside a class are separated by a single blank line.

Extra blank lines may be used (sparingly) to separate groups of related
functions.  Blank lines may be omitted between a bunch of related
one-liners (e.g. a set of dummy implementations).

Use blank lines in functions, sparingly, to indicate logical sections.

::: testing/docker/builder-kafkabroker/entrypoint.py
(Diff revision 2)
> +       return getattr(self.s, a)

E111: indentation is not a multiple of four
Use 4 spaces per indentation level.

For really old code that you don't want to mess up, you can continue to
use 8-space tabs.

::: testing/vcttesting/docker.py
(Diff revision 2)
> +                port = state['NetworkSettings']['Ports']['9092/tcp'][0]['HostPort']

E501: line too long (83 > 79 characters)
Limit all lines to a maximum of 79 characters.

There are still many devices around that are limited to 80 character
lines; plus, limiting windows to 80 characters makes it possible to have
several windows side-by-side.  The default wrapping on such devices looks
ugly.  Therefore, please limit all lines to a maximum of 79 characters.
For flowing long blocks of text (docstrings or comments), limiting the
length to 72 characters is recommended.

Reports error E501.

::: testing/vcttesting/docker_mach_commands.py
(Diff revision 2)
> +    @CommandArgument(
> +            '--shell', action='store_true',

E126: continuation line over-indented for hanging indent
Continuation lines indentation.

Continuation lines should align wrapped elements either vertically
using Python's implicit line joining inside parentheses, brackets
and braces, or using a hanging indent.

When using a hanging indent these considerations should be applied:
- there should be no arguments on the first line, and
- further indentation should be used to clearly distinguish itself as a
continuation line.

Comment 9

[Monty the Python Review Bot]

https://reviewboard.mozilla.org/r/3021/#review2353

And now for something completely different.

Congratulations, there we no Python static analysis issues with this patch!

The following files were examined:

  testing/vcttesting/docker.py

Comment 10

[Monty the Python Review Bot]

https://reviewboard.mozilla.org/r/3025/#review2355

Always look on the bright side of life.

I analyzed your Python changes and found 3 errors.

The following files were examined:

  pylib/vcsreplicator/setup.py
  pylib/vcsreplicator/vcsreplicator/__init__.py
  pylib/vcsreplicator/vcsreplicator/config.py
  pylib/vcsreplicator/vcsreplicator/hgext.py
  pylib/vcsreplicator/vcsreplicator/producer.py

::: pylib/vcsreplicator/vcsreplicator/config.py
(Diff revision 2)
> +    @property

E303: too many blank lines (2)
Separate top-level function and class definitions with two blank lines.

Method definitions inside a class are separated by a single blank line.

Extra blank lines may be used (sparingly) to separate groups of related
functions.  Blank lines may be omitted between a bunch of related
one-liners (e.g. a set of dummy implementations).

Use blank lines in functions, sparingly, to indicate logical sections.

::: pylib/vcsreplicator/vcsreplicator/config.py
(Diff revision 2)
> +            self._producer = Producer(
> +                    client, topic, partition, batch_send=False,

E126: continuation line over-indented for hanging indent
Continuation lines indentation.

Continuation lines should align wrapped elements either vertically
using Python's implicit line joining inside parentheses, brackets
and braces, or using a hanging indent.

When using a hanging indent these considerations should be applied:
- there should be no arguments on the first line, and
- further indentation should be used to clearly distinguish itself as a
continuation line.

::: pylib/vcsreplicator/vcsreplicator/producer.py
(Diff revision 2)
> +        return super(Producer, self).send_messages(
> +                self.topic, self.partition, msg)

E126: continuation line over-indented for hanging indent
Continuation lines indentation.

Continuation lines should align wrapped elements either vertically
using Python's implicit line joining inside parentheses, brackets
and braces, or using a hanging indent.

When using a hanging indent these considerations should be applied:
- there should be no arguments on the first line, and
- further indentation should be used to clearly distinguish itself as a
continuation line.

Comment 11

[Monty the Python Review Bot]

https://reviewboard.mozilla.org/r/3027/#review2357

And now for something completely different.

Congratulations, there we no Python static analysis issues with this patch!

The following files were examined:

  pylib/vcsreplicator/vcsreplicator/hgext.py
  pylib/vcsreplicator/vcsreplicator/producer.py

Comment 12

[Monty the Python Review Bot]

https://reviewboard.mozilla.org/r/3029/#review2359

Always look on the bright side of life.

I analyzed your Python changes and found 1 errors.

The following files were examined:

  pylib/vcsreplicator/vcsreplicator/config.py
  pylib/vcsreplicator/vcsreplicator/consumer.py

::: pylib/vcsreplicator/vcsreplicator/consumer.py
(Diff revision 2)
> +        super(Consumer, self).__init__(
> +                client, group, topic, partitions=partitions,

E126: continuation line over-indented for hanging indent
Continuation lines indentation.

Continuation lines should align wrapped elements either vertically
using Python's implicit line joining inside parentheses, brackets
and braces, or using a hanging indent.

When using a hanging indent these considerations should be applied:
- there should be no arguments on the first line, and
- further indentation should be used to clearly distinguish itself as a
continuation line.

Comment 13

[Monty the Python Review Bot]

https://reviewboard.mozilla.org/r/3045/#review2361

And now for something completely different.

Congratulations, there we no Python static analysis issues with this patch!

The following files were examined:

  pylib/vcsreplicator/vcsreplicator/config.py
  pylib/vcsreplicator/vcsreplicator/consumer.py

Comment 14

[Monty the Python Review Bot]

https://reviewboard.mozilla.org/r/3049/#review2363

And now for something completely different.

Congratulations, there we no Python static analysis issues with this patch!

The following files were examined:

  pylib/vcsreplicator/vcsreplicator/hgext.py

Comment 15

[Monty the Python Review Bot]

https://reviewboard.mozilla.org/r/3051/#review2365

Always look on the bright side of life.

I analyzed your Python changes and found 1 errors.

The following files were examined:

  pylib/vcsreplicator/vcsreplicator/consumer.py

::: pylib/vcsreplicator/vcsreplicator/consumer.py
(Diff revision 1)
> +    args = hglib.util.cmdbuilder(
> +            'init', path,

E126: continuation line over-indented for hanging indent
Continuation lines indentation.

Continuation lines should align wrapped elements either vertically
using Python's implicit line joining inside parentheses, brackets
and braces, or using a hanging indent.

When using a hanging indent these considerations should be applied:
- there should be no arguments on the first line, and
- further indentation should be used to clearly distinguish itself as a
continuation line.

Comment 16

[Monty the Python Review Bot]

https://reviewboard.mozilla.org/r/3057/#review2367

And now for something completely different.

Congratulations, there we no Python static analysis issues with this patch!

The following files were examined:

  pylib/vcsreplicator/vcsreplicator/hgext.py
  pylib/vcsreplicator/vcsreplicator/producer.py

Comment 17

[Gregory Szorc [:gps]]

Comment on attachment 8555046 [details]
MozReview Request: bz://1126153/gps

/r/3019 - testing: support for running a Kafka cluster
/r/3021 - testing: create topic as part of Kafka start
/r/3023 - testing: add kafka-python to virtualenv
/r/3025 - vcsreplicator: create project (bug 1126153)
/r/3027 - vcsreplicator: send a message when a repository is created
/r/3029 - vcsreplicator: implement basic consuming logic
/r/3045 - vcsreplicator: make path normalization more useful
/r/3047 - testing: add python-hglib to requirements
/r/3049 - vcsreplicator: ability to disable event production
/r/3051 - vcsreplicator: create new repositories when init message is received
/r/3053 - vcsreplicator: factor common configuration into shared script
/r/3055 - vcsreplicator: add more helper methods to helpers.sh
/r/3057 - vcsreplicator: ability to record hgrc files
/r/3065 - vcsreplicator: write hgrc files in consumer
/r/3067 - vcsreplicator: forbid local commits
/r/3069 - vcsreplicator: teach the consumer CLI how to skip records
/r/3071 - vcsreplicator: record changegroup addition
/r/3073 - vcsreplicator: add a mechanism to map repo paths to pull URLs
/r/3075 - vcsreplicator: pull repositories when a changegroup message is received

Pull down these commits:

hg pull review -r 668841ed2442d16ad21997d4d77354852226df50

Comment 18

[Monty the Python Review Bot]

https://reviewboard.mozilla.org/r/3019/#review2369

And now for something completely different.

Congratulations, there we no Python static analysis issues with this patch!

The following files were examined:

  testing/docker/builder-kafkabroker/entrypoint.py
  testing/docker/builder-kafkabroker/set-servers.py
  testing/vcttesting/docker.py
  testing/vcttesting/docker_mach_commands.py

Comment 19

[Monty the Python Review Bot]

https://reviewboard.mozilla.org/r/3021/#review2371

And now for something completely different.

Congratulations, there we no Python static analysis issues with this patch!

The following files were examined:

  testing/vcttesting/docker.py

Comment 20

[Monty the Python Review Bot]

https://reviewboard.mozilla.org/r/3025/#review2373

And now for something completely different.

Congratulations, there we no Python static analysis issues with this patch!

The following files were examined:

  pylib/vcsreplicator/setup.py
  pylib/vcsreplicator/vcsreplicator/__init__.py
  pylib/vcsreplicator/vcsreplicator/config.py
  pylib/vcsreplicator/vcsreplicator/hgext.py
  pylib/vcsreplicator/vcsreplicator/producer.py

Comment 21

[Monty the Python Review Bot]

https://reviewboard.mozilla.org/r/3027/#review2375

And now for something completely different.

Congratulations, there we no Python static analysis issues with this patch!

The following files were examined:

  pylib/vcsreplicator/vcsreplicator/hgext.py
  pylib/vcsreplicator/vcsreplicator/producer.py

Comment 22

[Monty the Python Review Bot]

https://reviewboard.mozilla.org/r/3029/#review2377

And now for something completely different.

Congratulations, there we no Python static analysis issues with this patch!

The following files were examined:

  pylib/vcsreplicator/vcsreplicator/config.py
  pylib/vcsreplicator/vcsreplicator/consumer.py

Comment 23

[Monty the Python Review Bot]

https://reviewboard.mozilla.org/r/3045/#review2379

And now for something completely different.

Congratulations, there we no Python static analysis issues with this patch!

The following files were examined:

  pylib/vcsreplicator/vcsreplicator/config.py
  pylib/vcsreplicator/vcsreplicator/consumer.py

Comment 24

[Monty the Python Review Bot]

https://reviewboard.mozilla.org/r/3049/#review2381

And now for something completely different.

Congratulations, there we no Python static analysis issues with this patch!

The following files were examined:

  pylib/vcsreplicator/vcsreplicator/hgext.py

Comment 25

[Monty the Python Review Bot]

https://reviewboard.mozilla.org/r/3051/#review2383

And now for something completely different.

Congratulations, there we no Python static analysis issues with this patch!

The following files were examined:

  pylib/vcsreplicator/vcsreplicator/consumer.py

Comment 26

[Monty the Python Review Bot]

https://reviewboard.mozilla.org/r/3057/#review2385

And now for something completely different.

Congratulations, there we no Python static analysis issues with this patch!

The following files were examined:

  pylib/vcsreplicator/vcsreplicator/hgext.py
  pylib/vcsreplicator/vcsreplicator/producer.py

Comment 27

[Monty the Python Review Bot]

https://reviewboard.mozilla.org/r/3065/#review2387

And now for something completely different.

Congratulations, there we no Python static analysis issues with this patch!

The following files were examined:

  pylib/vcsreplicator/vcsreplicator/consumer.py

Comment 28

[Monty the Python Review Bot]

https://reviewboard.mozilla.org/r/3067/#review2389

And now for something completely different.

Congratulations, there we no Python static analysis issues with this patch!

The following files were examined:

  pylib/vcsreplicator/vcsreplicator/hgext.py

Comment 29

[Monty the Python Review Bot]

https://reviewboard.mozilla.org/r/3069/#review2391

And now for something completely different.

Congratulations, there we no Python static analysis issues with this patch!

The following files were examined:

  pylib/vcsreplicator/vcsreplicator/consumer.py

Comment 30

[Monty the Python Review Bot]

https://reviewboard.mozilla.org/r/3071/#review2393

And now for something completely different.

Congratulations, there we no Python static analysis issues with this patch!

The following files were examined:

  pylib/vcsreplicator/vcsreplicator/hgext.py
  pylib/vcsreplicator/vcsreplicator/producer.py

Comment 31

[Monty the Python Review Bot]

https://reviewboard.mozilla.org/r/3073/#review2395

And now for something completely different.

Congratulations, there we no Python static analysis issues with this patch!

The following files were examined:

  pylib/vcsreplicator/vcsreplicator/config.py

Comment 32

[Monty the Python Review Bot]

https://reviewboard.mozilla.org/r/3075/#review2397

And now for something completely different.

Congratulations, there we no Python static analysis issues with this patch!

The following files were examined:

  pylib/vcsreplicator/vcsreplicator/consumer.py

Comment 33

[Gregory Szorc [:gps]]

Comment on attachment 8555046 [details]
MozReview Request: bz://1126153/gps

/r/3019 - testing: support for running a Kafka cluster
/r/3021 - testing: create topic as part of Kafka start
/r/3023 - testing: add kafka-python to virtualenv
/r/3025 - vcsreplicator: create project (bug 1126153)
/r/3027 - vcsreplicator: send a message when a repository is created
/r/3029 - vcsreplicator: implement basic consuming logic
/r/3045 - vcsreplicator: make path normalization more useful
/r/3047 - testing: add python-hglib to requirements
/r/3049 - vcsreplicator: ability to disable event production
/r/3051 - vcsreplicator: create new repositories when init message is received
/r/3053 - vcsreplicator: factor common configuration into shared script
/r/3055 - vcsreplicator: add more helper methods to helpers.sh
/r/3057 - vcsreplicator: ability to record hgrc files
/r/3065 - vcsreplicator: write hgrc files in consumer
/r/3067 - vcsreplicator: forbid local commits
/r/3069 - vcsreplicator: teach the consumer CLI how to skip records
/r/3071 - vcsreplicator: record changegroup addition
/r/3073 - vcsreplicator: add a mechanism to map repo paths to pull URLs
/r/3075 - vcsreplicator: pull repositories when a changegroup message is received
/r/3077 - vcsreplicator: record pushkey changes
/r/3079 - vcsreplicator: call pushkey when a pushkey message is received
/r/3081 - vcsreplicator: add test for phases
/r/3083 - vcsreplicator: tests for bookmark replication in a lagging client
/r/3085 - vcsreplicator: tests for phases replication in a lagging client
/r/3087 - vcsreplicator: command to import an existing repository into replication
/r/3089 - vcsreplicator: consumer handling for repo import

Pull down these commits:

hg pull review -r 7eb11dbbbc02003cb2fa102f5b4319b00fc14d7e

Comment 34

[Monty the Python Review Bot]

https://reviewboard.mozilla.org/r/3019/#review2405

And now for something completely different.

Congratulations, there we no Python static analysis issues with this patch!

The following files were examined:

  testing/docker/builder-kafkabroker/entrypoint.py
  testing/docker/builder-kafkabroker/set-servers.py
  testing/vcttesting/docker.py
  testing/vcttesting/docker_mach_commands.py

Comment 35

[Monty the Python Review Bot]

https://reviewboard.mozilla.org/r/3021/#review2407

And now for something completely different.

Congratulations, there we no Python static analysis issues with this patch!

The following files were examined:

  testing/vcttesting/docker.py

Comment 36

[Monty the Python Review Bot]

https://reviewboard.mozilla.org/r/3025/#review2409

And now for something completely different.

Congratulations, there we no Python static analysis issues with this patch!

The following files were examined:

  pylib/vcsreplicator/setup.py
  pylib/vcsreplicator/vcsreplicator/__init__.py
  pylib/vcsreplicator/vcsreplicator/config.py
  pylib/vcsreplicator/vcsreplicator/hgext.py
  pylib/vcsreplicator/vcsreplicator/producer.py

Comment 37

[Monty the Python Review Bot]

https://reviewboard.mozilla.org/r/3027/#review2411

And now for something completely different.

Congratulations, there we no Python static analysis issues with this patch!

The following files were examined:

  pylib/vcsreplicator/vcsreplicator/hgext.py
  pylib/vcsreplicator/vcsreplicator/producer.py

Comment 38

[Monty the Python Review Bot]

https://reviewboard.mozilla.org/r/3029/#review2413

And now for something completely different.

Congratulations, there we no Python static analysis issues with this patch!

The following files were examined:

  pylib/vcsreplicator/vcsreplicator/config.py
  pylib/vcsreplicator/vcsreplicator/consumer.py

Comment 39

[Monty the Python Review Bot]

https://reviewboard.mozilla.org/r/3045/#review2415

And now for something completely different.

Congratulations, there we no Python static analysis issues with this patch!

The following files were examined:

  pylib/vcsreplicator/vcsreplicator/config.py
  pylib/vcsreplicator/vcsreplicator/consumer.py

Comment 40

[Monty the Python Review Bot]

https://reviewboard.mozilla.org/r/3049/#review2417

And now for something completely different.

Congratulations, there we no Python static analysis issues with this patch!

The following files were examined:

  pylib/vcsreplicator/vcsreplicator/hgext.py

Comment 41

[Monty the Python Review Bot]

https://reviewboard.mozilla.org/r/3051/#review2419

And now for something completely different.

Congratulations, there we no Python static analysis issues with this patch!

The following files were examined:

  pylib/vcsreplicator/vcsreplicator/consumer.py

Comment 42

[Monty the Python Review Bot]

https://reviewboard.mozilla.org/r/3057/#review2421

And now for something completely different.

Congratulations, there we no Python static analysis issues with this patch!

The following files were examined:

  pylib/vcsreplicator/vcsreplicator/hgext.py
  pylib/vcsreplicator/vcsreplicator/producer.py

Comment 43

[Monty the Python Review Bot]

https://reviewboard.mozilla.org/r/3065/#review2423

And now for something completely different.

Congratulations, there we no Python static analysis issues with this patch!

The following files were examined:

  pylib/vcsreplicator/vcsreplicator/consumer.py

Comment 44

[Monty the Python Review Bot]

https://reviewboard.mozilla.org/r/3067/#review2425

And now for something completely different.

Congratulations, there we no Python static analysis issues with this patch!

The following files were examined:

  pylib/vcsreplicator/vcsreplicator/hgext.py

Comment 45

[Monty the Python Review Bot]

https://reviewboard.mozilla.org/r/3069/#review2427

And now for something completely different.

Congratulations, there we no Python static analysis issues with this patch!

The following files were examined:

  pylib/vcsreplicator/vcsreplicator/consumer.py

Comment 46

[Monty the Python Review Bot]

https://reviewboard.mozilla.org/r/3071/#review2429

And now for something completely different.

Congratulations, there we no Python static analysis issues with this patch!

The following files were examined:

  pylib/vcsreplicator/vcsreplicator/hgext.py
  pylib/vcsreplicator/vcsreplicator/producer.py

Comment 47

[Monty the Python Review Bot]

https://reviewboard.mozilla.org/r/3073/#review2431

And now for something completely different.

Congratulations, there we no Python static analysis issues with this patch!

The following files were examined:

  pylib/vcsreplicator/vcsreplicator/config.py

Comment 48

[Monty the Python Review Bot]

https://reviewboard.mozilla.org/r/3075/#review2433

And now for something completely different.

Congratulations, there we no Python static analysis issues with this patch!

The following files were examined:

  pylib/vcsreplicator/vcsreplicator/consumer.py

Comment 49

[Monty the Python Review Bot]

https://reviewboard.mozilla.org/r/3077/#review2435

And now for something completely different.

Congratulations, there we no Python static analysis issues with this patch!

The following files were examined:

  pylib/vcsreplicator/vcsreplicator/hgext.py
  pylib/vcsreplicator/vcsreplicator/producer.py

Comment 50

[Monty the Python Review Bot]

https://reviewboard.mozilla.org/r/3079/#review2437

And now for something completely different.

Congratulations, there we no Python static analysis issues with this patch!

The following files were examined:

  pylib/vcsreplicator/vcsreplicator/consumer.py

Comment 51

[Monty the Python Review Bot]

https://reviewboard.mozilla.org/r/3087/#review2439

And now for something completely different.

Congratulations, there we no Python static analysis issues with this patch!

The following files were examined:

  pylib/vcsreplicator/vcsreplicator/hgext.py
  pylib/vcsreplicator/vcsreplicator/producer.py

Comment 52

[Monty the Python Review Bot]

https://reviewboard.mozilla.org/r/3089/#review2441

And now for something completely different.

Congratulations, there we no Python static analysis issues with this patch!

The following files were examined:

  pylib/vcsreplicator/vcsreplicator/consumer.py

Comment 53

[Gregory Szorc [:gps]]

Comment on attachment 8555046 [details]
MozReview Request: bz://1126153/gps

/r/3019 - testing: support for running a Kafka cluster
/r/3021 - testing: create topic as part of Kafka start
/r/3023 - testing: add kafka-python to virtualenv
/r/3025 - vcsreplicator: create project (bug 1126153)
/r/3027 - vcsreplicator: send a message when a repository is created
/r/3029 - vcsreplicator: implement basic consuming logic
/r/3045 - vcsreplicator: make path normalization more useful
/r/3047 - testing: add python-hglib to requirements
/r/3049 - vcsreplicator: ability to disable event production
/r/3051 - vcsreplicator: create new repositories when init message is received
/r/3053 - vcsreplicator: factor common configuration into shared script
/r/3055 - vcsreplicator: add more helper methods to helpers.sh
/r/3057 - vcsreplicator: ability to record hgrc files
/r/3065 - vcsreplicator: write hgrc files in consumer
/r/3067 - vcsreplicator: forbid local commits
/r/3069 - vcsreplicator: teach the consumer CLI how to skip records
/r/3071 - vcsreplicator: record changegroup addition
/r/3073 - vcsreplicator: add a mechanism to map repo paths to pull URLs
/r/3075 - vcsreplicator: pull repositories when a changegroup message is received
/r/3077 - vcsreplicator: record pushkey changes
/r/3079 - vcsreplicator: call pushkey when a pushkey message is received
/r/3081 - vcsreplicator: add test for phases
/r/3083 - vcsreplicator: tests for bookmark replication in a lagging client
/r/3085 - vcsreplicator: tests for phases replication in a lagging client
/r/3087 - vcsreplicator: command to import an existing repository into replication
/r/3089 - vcsreplicator: consumer handling for repo import
/r/3191 - vcsreplicator: refactor configuration to be more useful
/r/3193 - docs: start documenting vcsreplicator

Pull down these commits:

hg pull review -r 51d6baa4123f11f37c60ca293b15d6eb3b751b41

Comment 54

[Monty the Python Review Bot]

https://reviewboard.mozilla.org/r/3019/#review2575

And now for something completely different.

Congratulations, there we no Python static analysis issues with this patch!

The following files were examined:

  testing/docker/builder-kafkabroker/entrypoint.py
  testing/docker/builder-kafkabroker/set-servers.py
  testing/vcttesting/docker.py
  testing/vcttesting/docker_mach_commands.py

Comment 55

[Monty the Python Review Bot]

https://reviewboard.mozilla.org/r/3021/#review2577

And now for something completely different.

Congratulations, there we no Python static analysis issues with this patch!

The following files were examined:

  testing/vcttesting/docker.py

Comment 56

[Monty the Python Review Bot]

https://reviewboard.mozilla.org/r/3025/#review2579

And now for something completely different.

Congratulations, there we no Python static analysis issues with this patch!

The following files were examined:

  pylib/vcsreplicator/setup.py
  pylib/vcsreplicator/vcsreplicator/__init__.py
  pylib/vcsreplicator/vcsreplicator/config.py
  pylib/vcsreplicator/vcsreplicator/hgext.py
  pylib/vcsreplicator/vcsreplicator/producer.py

Comment 57

[Monty the Python Review Bot]

https://reviewboard.mozilla.org/r/3027/#review2581

And now for something completely different.

Congratulations, there we no Python static analysis issues with this patch!

The following files were examined:

  pylib/vcsreplicator/vcsreplicator/hgext.py
  pylib/vcsreplicator/vcsreplicator/producer.py

Comment 58

[Monty the Python Review Bot]

https://reviewboard.mozilla.org/r/3029/#review2583

And now for something completely different.

Congratulations, there we no Python static analysis issues with this patch!

The following files were examined:

  pylib/vcsreplicator/vcsreplicator/config.py
  pylib/vcsreplicator/vcsreplicator/consumer.py

Comment 59

[Monty the Python Review Bot]

https://reviewboard.mozilla.org/r/3045/#review2585

And now for something completely different.

Congratulations, there we no Python static analysis issues with this patch!

The following files were examined:

  pylib/vcsreplicator/vcsreplicator/config.py
  pylib/vcsreplicator/vcsreplicator/consumer.py

Comment 60

[Monty the Python Review Bot]

https://reviewboard.mozilla.org/r/3049/#review2587

And now for something completely different.

Congratulations, there we no Python static analysis issues with this patch!

The following files were examined:

  pylib/vcsreplicator/vcsreplicator/hgext.py

Comment 61

[Monty the Python Review Bot]

https://reviewboard.mozilla.org/r/3051/#review2589

And now for something completely different.

Congratulations, there we no Python static analysis issues with this patch!

The following files were examined:

  pylib/vcsreplicator/vcsreplicator/consumer.py

Comment 62

[Monty the Python Review Bot]

https://reviewboard.mozilla.org/r/3057/#review2591

And now for something completely different.

Congratulations, there we no Python static analysis issues with this patch!

The following files were examined:

  pylib/vcsreplicator/vcsreplicator/hgext.py
  pylib/vcsreplicator/vcsreplicator/producer.py

Comment 63

[Monty the Python Review Bot]

https://reviewboard.mozilla.org/r/3065/#review2593

And now for something completely different.

Congratulations, there we no Python static analysis issues with this patch!

The following files were examined:

  pylib/vcsreplicator/vcsreplicator/consumer.py

Comment 64

[Monty the Python Review Bot]

https://reviewboard.mozilla.org/r/3067/#review2595

And now for something completely different.

Congratulations, there we no Python static analysis issues with this patch!

The following files were examined:

  pylib/vcsreplicator/vcsreplicator/hgext.py

Comment 65

[Monty the Python Review Bot]

https://reviewboard.mozilla.org/r/3069/#review2597

And now for something completely different.

Congratulations, there we no Python static analysis issues with this patch!

The following files were examined:

  pylib/vcsreplicator/vcsreplicator/consumer.py

Comment 66

[Monty the Python Review Bot]

https://reviewboard.mozilla.org/r/3071/#review2599

And now for something completely different.

Congratulations, there we no Python static analysis issues with this patch!

The following files were examined:

  pylib/vcsreplicator/vcsreplicator/hgext.py
  pylib/vcsreplicator/vcsreplicator/producer.py

Comment 67

[Monty the Python Review Bot]

https://reviewboard.mozilla.org/r/3073/#review2601

And now for something completely different.

Congratulations, there we no Python static analysis issues with this patch!

The following files were examined:

  pylib/vcsreplicator/vcsreplicator/config.py

Comment 68

[Monty the Python Review Bot]

https://reviewboard.mozilla.org/r/3075/#review2603

And now for something completely different.

Congratulations, there we no Python static analysis issues with this patch!

The following files were examined:

  pylib/vcsreplicator/vcsreplicator/consumer.py

Comment 69

[Monty the Python Review Bot]

https://reviewboard.mozilla.org/r/3077/#review2605

And now for something completely different.

Congratulations, there we no Python static analysis issues with this patch!

The following files were examined:

  pylib/vcsreplicator/vcsreplicator/hgext.py
  pylib/vcsreplicator/vcsreplicator/producer.py

Comment 70

[Monty the Python Review Bot]

https://reviewboard.mozilla.org/r/3079/#review2607

And now for something completely different.

Congratulations, there we no Python static analysis issues with this patch!

The following files were examined:

  pylib/vcsreplicator/vcsreplicator/consumer.py

Comment 71

[Monty the Python Review Bot]

https://reviewboard.mozilla.org/r/3087/#review2609

And now for something completely different.

Congratulations, there we no Python static analysis issues with this patch!

The following files were examined:

  pylib/vcsreplicator/vcsreplicator/hgext.py
  pylib/vcsreplicator/vcsreplicator/producer.py

Comment 72

[Monty the Python Review Bot]

https://reviewboard.mozilla.org/r/3089/#review2611

And now for something completely different.

Congratulations, there we no Python static analysis issues with this patch!

The following files were examined:

  pylib/vcsreplicator/vcsreplicator/consumer.py

Comment 73

[Monty the Python Review Bot]

https://reviewboard.mozilla.org/r/3191/#review2613

And now for something completely different.

Congratulations, there we no Python static analysis issues with this patch!

The following files were examined:

  pylib/vcsreplicator/vcsreplicator/consumer.py
  pylib/vcsreplicator/vcsreplicator/hgext.py

Comment 74

[Gregory Szorc [:gps]]

This is one of my Q4 deliverables. The servers are in a much happier place this time around and I'm confident this will get done.

I already have the hg.mo environment in Docker running a Kafka/ZooKeeper cluster. I've been hacking on this work in the past few months and am pretty confident we'll be able to start rolling out experiments soonish.

Comment 75

[Gregory Szorc [:gps]]

Attachment: MozReview Request: vcsreplicator: implement log-based replication mechanism for Mercurial (bug 1126153); r?dminor, r?smacleod

vcsreplicator: implement log-based replication mechanism for Mercurial (bug 1126153); r?dminor, r?smacleod

The behavior of the new system is detailed in the replication.rst file.

Comment 76

[Dan Minor [:dminor]]

Comment on attachment 8693872 [details]
MozReview Request: vcsreplicator: implement log-based replication mechanism for Mercurial (bug 1126153); r?dminor, r?smacleod

https://reviewboard.mozilla.org/r/26603/#review24289

This looks good to me. There's a lot to digest here, I'd like to have a second look before giving it a "ship it."

::: pylib/vcsreplicator/tests/test-check-consumer-lag.t:103
(Diff revision 1)
> +  $ check --warning-lag-time 1.0

I doubt it will ever matter, but I'd be happier if the lag-time was something like 0.9 rather than equal to the sleep time.

::: pylib/vcsreplicator/tests/test-check-consumer-lag.t:123
(Diff revision 1)
> +  $ check --critical-lag-time 1.0

again, I'd prefer 0.9 here.

::: pylib/vcsreplicator/tests/test-check-consumer-lag.t:153
(Diff revision 1)
> +  $ check --warning-lag-count 0 --warning-lag-time 1.0

Is there a guarantee that the consumers will be fully started before this check executes?

::: pylib/vcsreplicator/tests/test-consumer-daemon-restart.t:9
(Diff revision 1)
> +Gracefully shut down a consumer daemon

Do we expect to be able to restart a consumer if it did not shutdown cleany?

::: pylib/vcsreplicator/vcsreplicator/config.py:15
(Diff revision 1)
> +class Config(object):

Please add a docstring

::: pylib/vcsreplicator/vcsreplicator/config.py:73
(Diff revision 1)
> +                        timeout=connect_timeout)

nit: please indent under hosts

::: pylib/vcsreplicator/vcsreplicator/consumer.py:38
(Diff revision 1)
> +            max_buffer_size=104857600)

Please define this as a constant with a comment saying 100MB.

::: pylib/vcsreplicator/vcsreplicator/consumer.py:50
(Diff revision 1)
> +                get_partition_info=True)

nit: please indent under timeout

::: pylib/vcsreplicator/vcsreplicator/consumer.py:57
(Diff revision 1)
> +        if not d.startswith('1\n'):

Please add a constant which defines the message header.

::: pylib/vcsreplicator/vcsreplicator/consumer.py:272
(Diff revision 1)
> +                lag_time = time.time() - payload['_created']

Do you need to worry about the local clock getting out of sync with the producer's clock? Logging unexpected values here (e.g. negative lag_time) might help to detect this.

::: pylib/vcsreplicator/vcsreplicator/consumer.py:349
(Diff revision 1)
> +            help='Skip the consuming of the next message then exit')

A --verbose argument might be useful in conjunction with --onetime or --skip to dump the single message consumed or skipped when debugging tests.

::: pylib/vcsreplicator/vcsreplicator/hgext.py:80
(Diff revision 1)
> +    # TODO assert we're in a transaction.

Is there a reason this assertion can't be added now?

::: pylib/vcsreplicator/vcsreplicator/hgext.py:317
(Diff revision 1)
> +    class replicatingui(ui.__class__):

Please add a docstring

::: pylib/vcsreplicator/vcsreplicator/hgext.py:345
(Diff revision 1)
> +                if not k.startswith('partitionmap.'):

It would be nice to log or warn on this to help catch typos.

::: pylib/vcsreplicator/vcsreplicator/hgext.py:417
(Diff revision 1)
> +    class replicatingrepo(repo.__class__):

Please add a docstring.

::: pylib/vcsreplicator/vcsreplicator/hgext.py:471
(Diff revision 1)
> +                return parts[offset]

Having a record somewhere of which repo is assigned to which partition would help determine "occupancy" for each partition and whether or not the total number of partitions needs to be adjusted.

::: pylib/vcsreplicator/vcsreplicator/nagios.py:40
(Diff revision 1)
> +    exit = 0

nit: I'd prefer exitcode to exit

::: pylib/vcsreplicator/vcsreplicator/producer.py:38
(Diff revision 1)
> +        msg = b'1\n%s' % j

Please define a constant for the message type.

::: pylib/vcsreplicator/vcsreplicator/util.py:27
(Diff revision 1)
> +        # Don't pass topic name to function or it will attempt to create.

nit: "create" -> "create it"

Comment 77

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/26603/#review24289

> Is there a guarantee that the consumers will be fully started before this check executes?

In theory there is a race condition. But I've never seen it. I'll document it though.

> Do we expect to be able to restart a consumer if it did not shutdown cleany?

Yes. I tried to add a test but Docker isn't letting me kill -9 a process from inside a container. *sigh*

> Do you need to worry about the local clock getting out of sync with the producer's clock? Logging unexpected values here (e.g. negative lag_time) might help to detect this.

Yes, clock skew is a concern. Not sure if logging is going to matter here since we only run this as part of the lag Nagios check. We may want that Nagios check to warn if there is negative lag though.

> Having a record somewhere of which repo is assigned to which partition would help determine "occupancy" for each partition and whether or not the total number of partitions needs to be adjusted.

Since we're using a hash function that should be uniformally distributed, we shouldn't have any major concerns about occupancy. If we run into load problems, those will manifest as replication lag delays. If we get a lot of those, we can increase the partition count or adjust the partition map.

Comment 78

[Steven MacLeod [:smacleod]]

Comment on attachment 8693872 [details]
MozReview Request: vcsreplicator: implement log-based replication mechanism for Mercurial (bug 1126153); r?dminor, r?smacleod

https://reviewboard.mozilla.org/r/26603/#review24297

::: docs/hgmo/replication.rst:251
(Diff revision 1)
> +in. Extreme care must be practices when going down this road.

"practiced"

::: pylib/vcsreplicator/tests/test-corrupt-repo.t:115
(Diff revision 1)
> +Cleanuo

"Cleanup"

::: pylib/vcsreplicator/vcsreplicator/config.py:47
(Diff revision 1)
> +            if lower.startswith(source):

Shouldn't we be calling `.lower()` on source here? Does something else ensure source is always lower, or are you just assuming it will never be configured with uppercase?

::: pylib/vcsreplicator/vcsreplicator/consumer.py:147
(Diff revision 1)
> +    # We can't use hglib.init() because it doesn't pass config options
> +    # as part of the `hg init` call.

You aren't passing any either? What am I missing here? The only thing I can find would be `vcsreplicator.disableproduce=true` which I see where you set, but never where it is used?

Is it really because you don't want to open a connection to the command server?

::: pylib/vcsreplicator/vcsreplicator/consumer.py:188
(Diff revision 1)
> +def process_hg_pushkey(config, path, namespace, key, old, new, ret):

`ret` is unused, I guess with the command server we can't verify we get the same value? I'm assuming it doesn't really even matter though.

Can we possibly get into a case though where one of the pushkey operations returned some failure value but the hook still ran and we put it in the log? It appears that would cause problems here because rawcommand would throw.

I don't know of examples where we'd get failed pushkey operations on the writeable hg server, but if it happened we'd get a falsey value in 'ret', which I'm assuming would also mean a fail once we replicate - causing an exception here.

::: pylib/vcsreplicator/vcsreplicator/consumer.py:214
(Diff revision 1)
> +        logger.warn('pulling %d heads into %s' % (
> +            len(heads), local_path))
> +        c.pull(source=url or 'default')

We don't actually take heads into account at all here, did you just forget the rev argument to pull?

::: pylib/vcsreplicator/vcsreplicator/consumer.py:226
(Diff revision 1)
> +                      configs=['vcsreplicator.disableproduce=true'])

This disableproduce isn't used anywhere I can see...

::: pylib/vcsreplicator/vcsreplicator/consumer.py:331
(Diff revision 1)
> +    This does a couple of things. We can probably split it up into separate
> +    functions.

ya, this is kind of a strange pattern, heh.

::: pylib/vcsreplicator/vcsreplicator/hgext.py:75
(Diff revision 1)
> +    Pushkey updates should always occur inside a transaction. We don't write
> +    the pushkey update to the log inside the transaction because the
> +    transaction could get rolled back. Instead, we record the details of the
> +    pushkey and write messages after the transaction has closed.

From what I can tell pushkey *can* run when not in a transation (at least it checks if it's in one, and doesn't error when it's not) - I have no idea when this happens though.

Is there a reason you can guarantee it's in a transaction? Also, our hook runs when the lock is released, would the transaction have ended by that point?

Comment 79

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/26603/#review24297

> You aren't passing any either? What am I missing here? The only thing I can find would be `vcsreplicator.disableproduce=true` which I see where you set, but never where it is used?
> 
> Is it really because you don't want to open a connection to the command server?

The intent was to implement support for generaldelta, which it appears never got implemented *sigh*. I'll need to fix this.

Comment 80

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/26603/#review24297

> `ret` is unused, I guess with the command server we can't verify we get the same value? I'm assuming it doesn't really even matter though.
> 
> Can we possibly get into a case though where one of the pushkey operations returned some failure value but the hook still ran and we put it in the log? It appears that would cause problems here because rawcommand would throw.
> 
> I don't know of examples where we'd get failed pushkey operations on the writeable hg server, but if it happened we'd get a falsey value in 'ret', which I'm assuming would also mean a fail once we replicate - causing an exception here.

I'll fix this.

I haven't yet seen a non-0 pushkey return code in the wild. So I'm content with this being a fatal error (and worthy of human intervention).

> We don't actually take heads into account at all here, did you just forget the rev argument to pull?

Yes. Will fix.

> This disableproduce isn't used anywhere I can see...

This is left over from an earlier implementation. Good catch.

Comment 81

[Gregory Szorc [:gps]]

Comment on attachment 8693872 [details]
MozReview Request: vcsreplicator: implement log-based replication mechanism for Mercurial (bug 1126153); r?dminor, r?smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26603/diff/1-2/

Comment 82

[Gregory Szorc [:gps]]

Attachment: MozReview Request: hghooks: don't write message about legacy replication being disabled (bug 1126153); r?smacleod

We'll be removing the legacy replication mechanism shortly. To prepare
for this, remove some output saying the legacy replication system is
being used. This will make subsequent diffs smaller.

Review commit: https://reviewboard.mozilla.org/r/29507/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/29507/

Comment 83

[Gregory Szorc [:gps]]

Attachment: MozReview Request: hgserver: enable vcsreplicator by default (bug 1126153); r?smacleod

It has been running in production for several weeks and is mostly
working. It is time to enable it by default.

Some test output has changed because tests under hgserver were
previously relying on the legacy replication mechanism. However, the
test output reveals no functionality has changed, so yay.

Review commit: https://reviewboard.mozilla.org/r/29513/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/29513/

Comment 84

[Gregory Szorc [:gps]]

Attachment: MozReview Request: ansible/hg-ssh: remove hgrc-all (bug 1126153); r?smacleod

It is no longer needed since the new replication system is enabled by
default.

Review commit: https://reviewboard.mozilla.org/r/29515/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/29515/

Comment 85

[Gregory Szorc [:gps]]

Attachment: MozReview Request: hgserver: remove legacy test for vcsreplicator interaction (bug 1126153); r?smacleod

We'll shortly be removing the legacy replication hook. Remove this test
since it will shortly not be relevant. Also, this test likely had a race
condition after vcsreplicator was enabled by default.

Review commit: https://reviewboard.mozilla.org/r/29517/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/29517/

Comment 86

[Gregory Szorc [:gps]]

Attachment: MozReview Request: hgserver: remove activate-vcsreplicator script (bug 1126153); r?smacleod

The new replication system is enabled by default. We no longer need this
support script to enable it.

Review commit: https://reviewboard.mozilla.org/r/29519/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/29519/

Comment 87

[Gregory Szorc [:gps]]

Attachment: MozReview Request: hghooks: remove legacy replication hook (bug 1126153); r?smacleod

vcsreplicator is now handling replication. The legacy hook is no longer
needed.

Review commit: https://reviewboard.mozilla.org/r/29521/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/29521/

Comment 88

[Gregory Szorc [:gps]]

Attachment: MozReview Request: hgserver: replicate hgrc changes via vcsreplicator (bug 1126153); r?smacleod

Now that the new replication is enabled, we can integrate pash with it.
Start by having pash record hgrc changes via the new replication system.

The change adds explicit test coverage for hgrc content replication.
Before, it only verified the SSH server had its config updated.

Review commit: https://reviewboard.mozilla.org/r/29523/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/29523/

Comment 89

[Gregory Szorc [:gps]]

Attachment: MozReview Request: hgserver: replicate cloning via vcsreplicator (bug 1126153); r?smacleod

vcsreplicator doesn't currently intercept `hg clone` commands and
automatically generate replication events (it arguably should). So,
change how repository cloning is performed from `hg clone` to
`hg init` + `hg pull` + `hg replicatesync` so replication events are
written accordingly.

Also add in explicit testing that hgweb machines replicated the clone,
which was lacking before. This uncovered a bug in the lack of pash
creating per-user WSGI files. (This is done as an hourly CRON on the
servers to fill any gaps - but it should be done at repo creation time.)

Review commit: https://reviewboard.mozilla.org/r/29525/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/29525/

Comment 90

[Gregory Szorc [:gps]]

Attachment: MozReview Request: hgserver: don't explicitly replicate when creating empty user repo (bug 1126153); r?smacleod

`hg init` will send a replication message automatically. There is no
need to call into the legacy replication system to initiate the
replication.

Review commit: https://reviewboard.mozilla.org/r/29527/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/29527/

Comment 91

[Gregory Szorc [:gps]]

Attachment: MozReview Request: hgserver: inline do_repo_push; r?smacleod

We only have one consumer of this function now. Inline it and drop a
note to implement similar functionality in vcsreplicator.

Review commit: https://reviewboard.mozilla.org/r/29529/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/29529/

Comment 92

[Dan Minor [:dminor]]

Comment on attachment 8693872 [details]
MozReview Request: vcsreplicator: implement log-based replication mechanism for Mercurial (bug 1126153); r?dminor, r?smacleod

https://reviewboard.mozilla.org/r/26603/#review26345

Comment 93

[Gregory Szorc [:gps]]

Comment on attachment 8703946 [details]
MozReview Request: hgserver: enable vcsreplicator by default (bug 1126153); r?smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29513/diff/1-2/

Comment 94

[Gregory Szorc [:gps]]

Comment on attachment 8703947 [details]
MozReview Request: ansible/hg-ssh: remove hgrc-all (bug 1126153); r?smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29515/diff/1-2/

Comment 95

[Gregory Szorc [:gps]]

Comment on attachment 8703948 [details]
MozReview Request: hgserver: remove legacy test for vcsreplicator interaction (bug 1126153); r?smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29517/diff/1-2/

Comment 96

[Gregory Szorc [:gps]]

Comment on attachment 8703949 [details]
MozReview Request: hgserver: remove activate-vcsreplicator script (bug 1126153); r?smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29519/diff/1-2/

Comment 97

[Gregory Szorc [:gps]]

Comment on attachment 8703950 [details]
MozReview Request: hghooks: remove legacy replication hook (bug 1126153); r?smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29521/diff/1-2/

Comment 98

[Gregory Szorc [:gps]]

Comment on attachment 8703951 [details]
MozReview Request: hgserver: replicate hgrc changes via vcsreplicator (bug 1126153); r?smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29523/diff/1-2/

Comment 99

[Gregory Szorc [:gps]]

Comment on attachment 8703952 [details]
MozReview Request: hgserver: replicate cloning via vcsreplicator (bug 1126153); r?smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29525/diff/1-2/

Comment 100

[Gregory Szorc [:gps]]

Comment on attachment 8703953 [details]
MozReview Request: hgserver: don't explicitly replicate when creating empty user repo (bug 1126153); r?smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29527/diff/1-2/

Comment 101

[Gregory Szorc [:gps]]

Comment on attachment 8703954 [details]
MozReview Request: hgserver: inline do_repo_push; r?smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29529/diff/1-2/

Comment 102

[Gregory Szorc [:gps]]

Attachment: MozReview Request: ansible/hg-ssh: convert hgrc to a template (bug 1126153); r?smacleod

We'll be merging hgrc-vcsreplicator into hgrc and this requires it being
a template.

Review commit: https://reviewboard.mozilla.org/r/29589/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/29589/

Comment 103

[Gregory Szorc [:gps]]

Attachment: MozReview Request: ansible/hg-ssh: merge hgrc-vcsreplicator into hgrc (bug 1126153); r?smacleod

vcsreplicator is no longer optional. There is no good reason to have the
config exist in a separate file. Put everything in the main hgrc file.

Blackbox logging output changed slightly due to the order extensions are
loaded.

Review commit: https://reviewboard.mozilla.org/r/29591/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/29591/

Comment 104

[Gregory Szorc [:gps]]

Attachment: MozReview Request: docs: document modern replication mechanisms

Review commit: https://reviewboard.mozilla.org/r/29593/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/29593/

Comment 105

[Steven MacLeod [:smacleod]]

Comment on attachment 8693872 [details]
MozReview Request: vcsreplicator: implement log-based replication mechanism for Mercurial (bug 1126153); r?dminor, r?smacleod

https://reviewboard.mozilla.org/r/26603/#review26427

::: pylib/vcsreplicator/vcsreplicator/hgext.py:78
(Diff revisions 1 - 2)
> -    pushkey and write messages after the transaction has closed.
> +    # there may not be an active transaction# when we're called! However,

missing new-line? or extra `#`?

::: pylib/vcsreplicator/vcsreplicator/hgext.py:80
(Diff revisions 1 - 2)
> -    # TODO assert we're in a transaction.
> +    # do obtain a transaction. So a transaction is involved with pushkey
> +    # updates.

Thanks for clarifying all this :)

Comment 106

[Steven MacLeod [:smacleod]]

Comment on attachment 8703945 [details]
MozReview Request: hghooks: don't write message about legacy replication being disabled (bug 1126153); r?smacleod

https://reviewboard.mozilla.org/r/29507/#review26429

Comment 107

[Steven MacLeod [:smacleod]]

Comment on attachment 8703946 [details]
MozReview Request: hgserver: enable vcsreplicator by default (bug 1126153); r?smacleod

https://reviewboard.mozilla.org/r/29513/#review26431

Comment 108

[Steven MacLeod [:smacleod]]

Comment on attachment 8703947 [details]
MozReview Request: ansible/hg-ssh: remove hgrc-all (bug 1126153); r?smacleod

https://reviewboard.mozilla.org/r/29515/#review26433

Comment 109

[Steven MacLeod [:smacleod]]

Comment on attachment 8703948 [details]
MozReview Request: hgserver: remove legacy test for vcsreplicator interaction (bug 1126153); r?smacleod

https://reviewboard.mozilla.org/r/29517/#review26435

Comment 110

[Steven MacLeod [:smacleod]]

Comment on attachment 8703949 [details]
MozReview Request: hgserver: remove activate-vcsreplicator script (bug 1126153); r?smacleod

https://reviewboard.mozilla.org/r/29519/#review26437

Comment 111

[Steven MacLeod [:smacleod]]

Comment on attachment 8703950 [details]
MozReview Request: hghooks: remove legacy replication hook (bug 1126153); r?smacleod

https://reviewboard.mozilla.org/r/29521/#review26439

Comment 112

[Steven MacLeod [:smacleod]]

Comment on attachment 8703951 [details]
MozReview Request: hgserver: replicate hgrc changes via vcsreplicator (bug 1126153); r?smacleod

https://reviewboard.mozilla.org/r/29523/#review26441

::: hgserver/tests/test-obsolescence.t:55
(Diff revision 2)
> +  $ hgmo exec hgweb0 cat /repo/hg/mozilla/users/user_example.com/repo-1/.hg/hgrc
> +  [phases]
> +  publish = False
> +  
> +  [experimental]
> +  evolution = all
> +  

Where is the replication of this actually tested??

Comment 113

[Steven MacLeod [:smacleod]]

https://reviewboard.mozilla.org/r/29523/#review26441

> Where is the replication of this actually tested??

Totally misread, ignore me :)

Comment 114

[Steven MacLeod [:smacleod]]

Comment on attachment 8703952 [details]
MozReview Request: hgserver: replicate cloning via vcsreplicator (bug 1126153); r?smacleod

https://reviewboard.mozilla.org/r/29525/#review26445

Comment 115

[Steven MacLeod [:smacleod]]

Comment on attachment 8703953 [details]
MozReview Request: hgserver: don't explicitly replicate when creating empty user repo (bug 1126153); r?smacleod

https://reviewboard.mozilla.org/r/29527/#review26447

Comment 116

[Steven MacLeod [:smacleod]]

Comment on attachment 8703954 [details]
MozReview Request: hgserver: inline do_repo_push; r?smacleod

https://reviewboard.mozilla.org/r/29529/#review26449

Comment 117

[Steven MacLeod [:smacleod]]

Comment on attachment 8704257 [details]
MozReview Request: ansible/hg-ssh: convert hgrc to a template (bug 1126153); r?smacleod

https://reviewboard.mozilla.org/r/29589/#review26451

Comment 118

[Steven MacLeod [:smacleod]]

Comment on attachment 8704258 [details]
MozReview Request: ansible/hg-ssh: merge hgrc-vcsreplicator into hgrc (bug 1126153); r?smacleod

https://reviewboard.mozilla.org/r/29591/#review26453

::: ansible/roles/hg-ssh/templates/hgrc.j2:51
(Diff revision 1)
> +# This is a standalone config file to configure the vcsreplicator.
> +# It can be included by individual repositories to test vcsreplicator.

This is nonsense now.

Comment 119

[Steven MacLeod [:smacleod]]

Comment on attachment 8704259 [details]
MozReview Request: docs: document modern replication mechanisms

https://reviewboard.mozilla.org/r/29593/#review26455

Comment 120

[Gregory Szorc [:gps]]

https://hg.mozilla.org/hgcustom/version-control-tools/rev/f5b16e3c35931f8ecbe781e274961b61e838c6bb
hghooks: don't write message about legacy replication being disabled (bug 1126153); r=smacleod

https://hg.mozilla.org/hgcustom/version-control-tools/rev/4dc4d58314ce9b8de9aa08737fd8de887c54e0bf
hgserver: enable vcsreplicator by default (bug 1126153); r=smacleod

https://hg.mozilla.org/hgcustom/version-control-tools/rev/cf1e9668ea7055d3b869b1f2ec305aedf4e6dbd2
ansible/hg-ssh: remove hgrc-all (bug 1126153); r=smacleod

https://hg.mozilla.org/hgcustom/version-control-tools/rev/428601fef782a763f2227b47545febae5637ea71
hgserver: remove legacy test for vcsreplicator interaction (bug 1126153); r=smacleod

https://hg.mozilla.org/hgcustom/version-control-tools/rev/dccd6cc30713047212d8cb55b82dceaa54c8a8f7
hgserver: remove activate-vcsreplicator script (bug 1126153); r=smacleod

https://hg.mozilla.org/hgcustom/version-control-tools/rev/6bbefc64099255fa79fac79031ca25c65addee19
hghooks: remove legacy replication hook (bug 1126153); r=smacleod

https://hg.mozilla.org/hgcustom/version-control-tools/rev/47cda311bd29550eca5bad017eeadaa0c37790b5
hgserver: replicate hgrc changes via vcsreplicator (bug 1126153); r=smacleod

https://hg.mozilla.org/hgcustom/version-control-tools/rev/5478db0f544849ca5ea3f268877ca1d3eea7788e
hgserver: replicate cloning via vcsreplicator (bug 1126153); r=smacleod

https://hg.mozilla.org/hgcustom/version-control-tools/rev/71d2509445b53550bb3f4da3b742d46d0bb2c90b
hgserver: don't explicitly replicate when creating empty user repo (bug 1126153); r=smacleod

https://hg.mozilla.org/hgcustom/version-control-tools/rev/954c87148af9b9b6d612fa73dc11da919beaffc2
ansible/hg-ssh: convert hgrc to a template (bug 1126153); r=smacleod

https://hg.mozilla.org/hgcustom/version-control-tools/rev/fd5d34a50a45972919e8ace0cbf7e8eae50b6b62
ansible/hg-ssh: merge hgrc-vcsreplicator into hgrc (bug 1126153); r=smacleod

Comment 121

[Gregory Szorc [:gps]]

I'm considering this one done.

There is still some minor work to update MOC docs for alerts. It's on my TODO list.