Skip to content

[IOTDB-1084] Fix temporary memory of flushing may cause OOM#2358

Merged
JackieTien97 merged 12 commits intomasterfrom
flush_OOM
Jan 19, 2021
Merged

[IOTDB-1084] Fix temporary memory of flushing may cause OOM#2358
JackieTien97 merged 12 commits intomasterfrom
flush_OOM

Conversation

@HTHou
Copy link
Contributor

@HTHou HTHou commented Dec 28, 2020


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
Contributor

@SilverNarcissus SilverNarcissus left a comment

Choose a reason for hiding this comment

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

Look good to me~

Copy link
Contributor

@SilverNarcissus SilverNarcissus left a comment

Choose a reason for hiding this comment

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

Fantastic work! But there are some issues, please have a look.

@@ -170,94 +189,104 @@ private void writeOneSeries(TVList tvPairs, IChunkWriter seriesWriterImpl,
@Override
public void run() {
long memSerializeTime = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

You should use class variable memSerializeTime rather than this local variable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed~

private volatile boolean noMoreEncodingTask = false;
private volatile boolean noMoreIOTask = false;
private long memSerializeTime = 0L;
private long ioTime = 0L;
Copy link
Contributor

@SilverNarcissus SilverNarcissus Jan 19, 2021

Choose a reason for hiding this comment

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

These two variable will be written and read by different thread without locking. The situation is only one thread to write, so no locking is reasonable, but they should be volatile so they can be seen by another thread.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed~


@SuppressWarnings("squid:S135")
private Runnable ioTask = () -> {
long ioTime = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

You should use class variable ioTime rather than this local variable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed~

Copy link
Contributor

@SilverNarcissus SilverNarcissus left a comment

Choose a reason for hiding this comment

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

Look good to me~

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

warning The version of Java (1.8.0_252) you have used to run this analysis is deprecated and we will stop accepting it soon. Please update to at least Java 11.
Read more here

@JackieTien97 JackieTien97 merged commit a07b5ec into master Jan 19, 2021
@HTHou HTHou deleted the flush_OOM branch January 20, 2021 01:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants