Hi @lbarbier,

I see your point: if the server is at the end of a chain of servers, then uncompressed journals are just consuming disk space.

I'm glad to hear that the 'p4 admin checkpoint' is working in your environment. I'm actually testing out a change at would use that logic for and P4D, falling back to calling 'p4d -jc' if the 'p4 admin' command failed.

The behaviour you're seeing with rotations starting at the master, and checkpoints being scheduled is by design. The journal number is stored in the counters tables, and replicated as part of the journal, so in order to ensure that journals and checkpoints match up between replicas, the journal rotation on the master causes journal rotations on the replicas. If you rotated the journal on just a replica, it would have a different journal number to the master. If the journal numbers somehow got out of sync, and you had some sort of failure that required you to restore the master, you would have no reference point to restore and restart the replica.

I agree that this could be made better, possibly by improving visibility of which replicas were scheduled to checkpoint, etc, but journal rotations are relatively cheap, so even if you rotated before you scheduled the checkpoint, and again after, I believe the overall time that the process will take will be about the same.

I'll update the review tomorrow with an option to compress the journal. I'm also going to move it into the main branch: I'll need to backport if I it's to be included in a patch release. I'll also include the 'p4 admin checkpoint/journal' calls that I mentioned (the only real difference to your shelf is that they are on the P4D class: I'm not convinced that we need another server type, and I'd rather not add unnecessary configuration options).

Thanks,

Nick

Hi @lbarbier,

Thanks for your continued work on this.

I've been thing a bit more about both cases, and I think we might want to consider a different direction:
ever since replication was introduced, compressing journals has become a problem. p4dctl's checkpoint and journal commands predate replication (they were part of the original open-source project: //guest/tony_smith/perforce/p4dctl/).

I propose that we don't add an option to rotate journals at all; instead, just change the '-z' flag to '-Z'. I still think that the option to enable/disable the flag is a great idea, and this is very similar to your original review.
I'll update the review shortly with this proposal.

As for supporting replica Perforce Servers, I think that running 'p4 admin checkpoint' from p4dctl might be problematic: p4dctl needs to be configured to run the command as a privileged user, potentially with authentication. Unlike the P4D::Stop() implementation, there's no good fallback for this at the moment.

I think we could adapt both P4D::Checkpoint() and P4D::Journal() to attempt to execute the 'p4 admin' commands, and fall back to calling 'p4d -jc/-jj'. If we added some error handling to detect the replica error message we could return a more useful message to the user.

To improve the experience even further, I'll log an enhancement request for 'p4d -jc' against a replica to schedule a checkpoint, so that if 'p4dctl checkpoint' falls back to running that command against a replica, it will still succeed.

Hi @lbarbier,

The issue with compressing only the journal (not the checkpoint) is that there's no p4d flag to do that. We could compress the rotated journal afterwards, or uncompress the checkpoint, but I think it's an inadvisable enough case that we don't need to worry about it.

I'll go ahead and make the changes in the review; I didn't want to go ahead without checking first, in case I caused offence.

As for checkpointing replica servers, that could be tricky, but it should be possible to identify if a server is acting as a replica and do something appropriate. I'll do some research and let you know what I come up with.

Hi @lbarbier

First off, thank you for contributing your solution back to the community.
This is a great start and I've got a couple of suggestions that will make this feature even better.

The issue you've encountered is that 'p4dctl checkpoint' is calling 'p4d -z -jc', which is compressing the checkpoint and the rotated journal, breaking replication. But I believe that you'd also encounter the same problem if you ran 'p4dctl journal', which rotates the journal by calling 'p4d -z -jj'.

There are two solutions to this problem:

1. Disable compression by not passing the '-z' flag (as in your shelf).
2. Compress only the checkpoint, not the journal, by passing the '-Z' flag.

I think there are probably use cases for both, so rather than providing a single compression on/off we should add two options:
CompressCheckpoint = true/false
CompressJournal = false/true

In this the case that CompressCheckpoint == true and CompressJournal == true, pass '-z'.
In this the case that CompressCheckpoint == true and CompressJournal == false, pass '-Z'.
In this the case that CompressCheckpoint == false and CompressJournal == false, pass no flag.
In this the case that CompressCheckpoint == false and CompressJournal == true, pass no flag, or raise a config parse error.

What do you think?

If you like, I can add my suggested changes to the review for you to test out, or if you'd prefer to make the change, I'll be happy to review it.
I'd like to work with you, so I'm happy to work whichever way you'd prefer.