Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Memory Threshold #14597

Merged
merged 44 commits into from
Jan 10, 2025
Merged

Add Memory Threshold #14597

merged 44 commits into from
Jan 10, 2025

Conversation

SpriCoder
Copy link
Contributor

@SpriCoder SpriCoder commented Dec 30, 2024

Add Memory Threshold Metrics
image

Copy link

codecov bot commented Dec 30, 2024

Codecov Report

Attention: Patch coverage is 0.64935% with 306 lines in your changes missing coverage. Please review.

Project coverage is 39.28%. Comparing base (a4ce6f7) to head (3784035).
Report is 16 commits behind head on master.

Files with missing lines Patch % Lines
...ice/metrics/memory/StorageEngineMemoryMetrics.java 0.00% 95 Missing ⚠️
...rvice/metrics/memory/QueryEngineMemoryMetrics.java 0.00% 74 Missing ⚠️
...db/service/metrics/memory/GlobalMemoryMetrics.java 0.00% 54 Missing ⚠️
...vice/metrics/memory/SchemaEngineMemoryMetrics.java 0.00% 46 Missing ⚠️
...service/metrics/memory/ConsensusMemoryMetrics.java 0.00% 18 Missing ⚠️
...vice/metrics/memory/StreamEngineMemoryMetrics.java 0.00% 18 Missing ⚠️
...otdb/db/service/metrics/DataNodeMetricsHelper.java 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master   #14597      +/-   ##
============================================
- Coverage     39.33%   39.28%   -0.05%     
  Complexity      193      193              
============================================
  Files          4379     4385       +6     
  Lines        277511   277882     +371     
  Branches      33881    33911      +30     
============================================
+ Hits         109158   109177      +19     
- Misses       168353   168705     +352     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@SpriCoder SpriCoder changed the title Add Write Memory Threshold Add Memory Threshold Jan 5, 2025
@SpriCoder SpriCoder marked this pull request as ready for review January 9, 2025 04:58
@@ -515,7 +509,7 @@ public void createDataRegionMemoryCostMetrics(DataRegion dataRegion) {
MetricService.getInstance()
.createAutoGauge(
Metric.DATA_REGION_MEM_COST.toString(),
MetricLevel.IMPORTANT,
MetricLevel.NORMAL,
Copy link
Contributor

Choose a reason for hiding this comment

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

Do not change existing metrics's level to normal?


public class IoTDBRuntimeOutOfMemoryException extends RuntimeException {

private final long timeStamp;
Copy link
Contributor

Choose a reason for hiding this comment

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

use timestamp

import java.nio.ByteBuffer;
import java.util.Objects;

public class IoTDBRuntimeOutOfMemoryException extends RuntimeException {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not actually an OutOfMemoryException, right? Maybe a more reasonable name could be changed

return memoryUsageInBytes.get();
}

public void setMaxMemorySizeInByte(final long maxMemorySizeInByte) {
Copy link
Contributor

Choose a reason for hiding this comment

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

mobe to line 39?

Copy link
Contributor

Choose a reason for hiding this comment

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

When will this function be called?

@Override
public boolean useMemory(final long size) {
if (size <= 0) {
memoryUsageInBytes.addAndGet(-size);
Copy link
Contributor

Choose a reason for hiding this comment

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

when will size be negetive?

import java.util.concurrent.atomic.AtomicLong;
import java.util.concurrent.locks.ReentrantLock;

public abstract class IIoTDBMemoryBlock implements AutoCloseable {
Copy link
Contributor

Choose a reason for hiding this comment

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

IMemoryBlock

Copy link
Contributor

Choose a reason for hiding this comment

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

Add some javadoc and description for this basic class

public abstract class IIoTDBMemoryBlock implements AutoCloseable {
protected IoTDBMemoryManager memoryManager;
protected IoTDBMemoryBlockType memoryBlockType;
protected final ReentrantLock lock = new ReentrantLock();
Copy link
Contributor

Choose a reason for hiding this comment

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

if you use this only in close method, why not directly use method synchronized?

Copy link

sonarqubecloud bot commented Jan 9, 2025

Copy link
Contributor

@OneSizeFitsQuorum OneSizeFitsQuorum left a comment

Choose a reason for hiding this comment

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

LGTM

@OneSizeFitsQuorum OneSizeFitsQuorum merged commit 01e1420 into master Jan 10, 2025
36 of 38 checks passed
@OneSizeFitsQuorum OneSizeFitsQuorum deleted the feature/optimize_memory branch January 10, 2025 11:20
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.

2 participants