Skip to content

[IOTDB-4110] customize trigger snapshot#7286

Closed
sollhui wants to merge 6 commits intoapache:masterfrom
sollhui:take-snapshot
Closed

[IOTDB-4110] customize trigger snapshot#7286
sollhui wants to merge 6 commits intoapache:masterfrom
sollhui:take-snapshot

Conversation

@sollhui
Copy link

@sollhui sollhui commented Sep 9, 2022

Description

Content1 ...

Content2 ...

Content3 ...


This PR has:

  • been self-reviewed.
    • concurrent read
    • concurrent write
    • concurrent read and write
  • added documentation for new or modified features or behaviors.
  • added Javadocs for most classes and all non-trivial methods.
  • added or updated version, license, or notice information
  • added comments explaining the "why" and the intent of the code wherever would not be obvious
    for an unfamiliar reader.
  • added unit tests or modified existing tests to cover new code paths, ensuring the threshold
    for code coverage.
  • added integration tests.
  • been tested in a test IoTDB cluster.

Key changed/added classes (or packages if there are too many classes) in this PR

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, this is your first pull request in IoTDB project. Thanks for your contribution! IoTDB will be better because of you.

Copy link
Member

@SzyWilliam SzyWilliam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this! There are some issues need to be fixed, PTAL. Also, use mvn spotless to format code before push.

private int retentionFileNum = RaftServerConfigKeys.Snapshot.RETENTION_FILE_NUM_DEFAULT;
private long triggerSnapshotTime = 60;
private long triggerSnapshotFileSize = 20L << 30;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add comments here. 60 seconds and 20 GB.

.getCurrentDir();
}
catch (IOException e) {
failed(new RatisGetDivisionException(e));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

failed(...) is called when construct a fail reply message to client. Just use log.warn here to warn user of GetDivisionException.

failed(new RatisGetDivisionException(e));
}

long currentDirLength = currentDir.length();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

File.length() will return

Returns the length of the file denoted by this abstract pathname. The return value is unspecified if this pathname denotes a directory.

Not the directory total size. Please refer to java doc https://docs.oracle.com/javase/8/docs/api/java/io/File.html#length--


ScheduledExecutorUtil.safelyScheduleWithFixedDelay(
executor,
command,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use this::triggerSnapshotByCustomize instead of lambda.

return ConsensusGenericResponse.newBuilder().setSuccess(reply.isSuccess()).build();
}

private void triggerSnapshotByCustomize(ConsensusConfig config) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we should add synchronized to method signature. Consider this: this method is not finished when 60s past and another call started.

@SzyWilliam
Copy link
Member

@HHoflittlefish777 Thanks for the update. Could you please see if you can add some unit tests to verify this?

Copy link
Member

@SzyWilliam SzyWilliam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@sollhui sollhui closed this Jul 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants