JackieTien97 commented on code in PR #12177:
URL: https://github.com/apache/iotdb/pull/12177#discussion_r1525967322
##########
iotdb-core/tsfile/src/main/java/org/apache/iotdb/tsfile/read/common/Path.java:
##########
@@ -139,6 +149,11 @@ public String getDevice() {
return device;
}
+ public IDeviceID getIDeviceID() {
+ // TODO
Review Comment:
TODO what?
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/queryengine/execution/operator/source/SeriesScanUtil.java:
##########
@@ -1314,7 +1315,7 @@ public boolean isOverlapped(long time, Statistics right) {
@Override
public boolean isOverlapped(long time, TsFileResource right) {
- return time <= right.getEndTime(seriesPath.getDevice());
+ return time <= right.getEndTime(new
PlainDeviceID(seriesPath.getDevice()));
Review Comment:
```suggestion
return time <= right.getEndTime(seriesPath.getIDeviceID());
```
##########
iotdb-core/tsfile/src/main/java/org/apache/iotdb/tsfile/read/common/Path.java:
##########
@@ -50,6 +52,10 @@ public class Path implements Serializable, Comparable<Path> {
public Path() {}
+ public Path(IDeviceID deviceID) {
+ this(((PlainDeviceID) deviceID).toStringID());
Review Comment:
When we need to change IDeviceID back to Path?
##########
iotdb-core/tsfile/src/main/java/org/apache/iotdb/tsfile/file/metadata/PlainDeviceID.java:
##########
@@ -38,6 +45,9 @@ public boolean equals(Object o) {
return true;
}
if (!(o instanceof PlainDeviceID)) {
+ if (o != null) {
+ new RuntimeException().printStackTrace();
+ }
Review Comment:
```suggestion
```
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/storageengine/dataregion/HashLastFlushTimeMap.java:
##########
@@ -65,20 +67,21 @@ public class HashLastFlushTimeMap implements
ILastFlushTimeMap {
*
* <p>It is used to update last cache.
*/
- private final Map<String, Long> globalLatestFlushedTimeForEachDevice = new
ConcurrentHashMap<>();
+ private final Map<IDeviceID, Long> globalLatestFlushedTimeForEachDevice =
+ new ConcurrentHashMap<>();
/** record memory cost of map for each partitionId */
private final Map<Long, Long> memCostForEachPartition = new
ConcurrentHashMap<>();
// For load
@Override
- public void updateOneDeviceFlushedTime(long timePartitionId, String
deviceId, long time) {
+ public void updateOneDeviceFlushedTime(long timePartitionId, IDeviceID
deviceId, long time) {
ILastFlushTime flushTimeMapForPartition =
partitionLatestFlushedTime.computeIfAbsent(
timePartitionId, id -> new DeviceLastFlushTime());
long lastFlushTime = flushTimeMapForPartition.getLastFlushTime(deviceId);
if (lastFlushTime == Long.MIN_VALUE) {
- long memCost = HASHMAP_NODE_BASIC_SIZE + 2L * deviceId.length();
+ long memCost = HASHMAP_NODE_BASIC_SIZE + 2L * deviceId.memorySize();
Review Comment:
memorySize() still need to be multiplied by 2?
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/storageengine/dataregion/DataRegion.java:
##########
@@ -2086,14 +2088,17 @@ private boolean canSkipDelete(
return false;
}
Pair<Long, Long> startAndEndTime =
- tsFileResource.getPossibleStartTimeAndEndTime(device,
deviceMatchInfo);
+ tsFileResource.getPossibleStartTimeAndEndTime(
+ device,
+
deviceMatchInfo.stream().map(PlainDeviceID::new).collect(Collectors.toSet()));
if (startAndEndTime == null) {
continue;
}
deviceStartTime = startAndEndTime.getLeft();
deviceEndTime = startAndEndTime.getRight();
} else {
- String deviceId = device.getFullPath();
+ // TODO: DELETE
Review Comment:
still need TODO?
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/storageengine/dataregion/compaction/execute/utils/writer/AbstractCompactionWriter.java:
##########
@@ -309,7 +311,9 @@ protected long getChunkSize(Chunk chunk) {
protected void checkPreviousTimestamp(long currentWritingTimestamp, int
subTaskId) {
if (currentWritingTimestamp <= lastTime[subTaskId]) {
throw new CompactionLastTimeCheckFailedException(
- deviceId + IoTDBConstant.PATH_SEPARATOR + measurementId[subTaskId],
+ ((PlainDeviceID) deviceId).toStringID()
Review Comment:
Why we need to cast it to `PlainDeviceID`? `toStringID()` is already
declared in `IDeviceID`
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/storageengine/dataregion/HashLastFlushTimeMap.java:
##########
@@ -87,15 +90,16 @@ public void updateOneDeviceFlushedTime(long
timePartitionId, String deviceId, lo
// For recover
@Override
- public void updateMultiDeviceFlushedTime(long timePartitionId, Map<String,
Long> flushedTimeMap) {
+ public void updateMultiDeviceFlushedTime(
+ long timePartitionId, Map<IDeviceID, Long> flushedTimeMap) {
ILastFlushTime flushTimeMapForPartition =
partitionLatestFlushedTime.computeIfAbsent(
timePartitionId, id -> new DeviceLastFlushTime());
long memIncr = 0;
- for (Map.Entry<String, Long> entry : flushedTimeMap.entrySet()) {
+ for (Map.Entry<IDeviceID, Long> entry : flushedTimeMap.entrySet()) {
if (flushTimeMapForPartition.getLastFlushTime(entry.getKey()) ==
Long.MIN_VALUE) {
- memIncr += HASHMAP_NODE_BASIC_SIZE + 2L * entry.getKey().length();
+ memIncr += HASHMAP_NODE_BASIC_SIZE + 2L * entry.getKey().memorySize();
Review Comment:
memorySize() still need to be multiplied by 2?
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/storageengine/dataregion/tsfile/timeindex/DeviceTimeIndex.java:
##########
@@ -212,19 +215,35 @@ public boolean stillLives(long ttlLowerBound) {
@Override
public long calculateRamSize() {
return INSTANCE_SIZE
- + RamUsageEstimator.sizeOfMap(
- deviceToIndex,
RamUsageEstimator.shallowSizeOfInstance(Integer.class))
+ +
calculateDeviceToIndexMapSize(RamUsageEstimator.shallowSizeOfInstance(Integer.class))
+ RamUsageEstimator.sizeOf(startTimes)
+ RamUsageEstimator.sizeOf(endTimes);
}
- private int getDeviceIndex(String deviceId) {
+ private long calculateDeviceToIndexMapSize(long defSize) {
+ if (deviceToIndex == null) {
+ return 0;
+ }
+ long size = RamUsageEstimator.shallowSizeOf(deviceToIndex);
+ long sizeOfEntry = -1;
+ for (Map.Entry<IDeviceID, ?> entry : deviceToIndex.entrySet()) {
+ if (sizeOfEntry == -1) {
+ sizeOfEntry = RamUsageEstimator.shallowSizeOf(entry);
+ }
+ size += sizeOfEntry;
+ size += entry.getKey().memorySize();
+ size += RamUsageEstimator.sizeOfObject(entry.getValue(), defSize);
+ }
+ return RamUsageEstimator.alignObjectSize(size);
+ }
Review Comment:
add` else if ` block in `sizeOfObject` method of `RamUsageEstimator` instead
of copying it here.
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/storageengine/dataregion/tsfile/timeindex/DeviceTimeIndex.java:
##########
@@ -212,19 +215,35 @@ public boolean stillLives(long ttlLowerBound) {
@Override
public long calculateRamSize() {
return INSTANCE_SIZE
- + RamUsageEstimator.sizeOfMap(
- deviceToIndex,
RamUsageEstimator.shallowSizeOfInstance(Integer.class))
+ +
calculateDeviceToIndexMapSize(RamUsageEstimator.shallowSizeOfInstance(Integer.class))
+ RamUsageEstimator.sizeOf(startTimes)
+ RamUsageEstimator.sizeOf(endTimes);
}
- private int getDeviceIndex(String deviceId) {
+ private long calculateDeviceToIndexMapSize(long defSize) {
+ if (deviceToIndex == null) {
+ return 0;
+ }
+ long size = RamUsageEstimator.shallowSizeOf(deviceToIndex);
+ long sizeOfEntry = -1;
+ for (Map.Entry<IDeviceID, ?> entry : deviceToIndex.entrySet()) {
+ if (sizeOfEntry == -1) {
+ sizeOfEntry = RamUsageEstimator.shallowSizeOf(entry);
+ }
+ size += sizeOfEntry;
+ size += entry.getKey().memorySize();
+ size += RamUsageEstimator.sizeOfObject(entry.getValue(), defSize);
+ }
+ return RamUsageEstimator.alignObjectSize(size);
+ }
+
+ private int getDeviceIndex(IDeviceID deviceId) {
int index;
if (deviceToIndex.containsKey(deviceId)) {
index = deviceToIndex.get(deviceId);
} else {
index = deviceToIndex.size();
-
deviceToIndex.put(DataNodeDevicePathCache.getInstance().getDeviceId(deviceId),
index);
+ deviceToIndex.put(deviceId, index);
Review Comment:
why we don't use the Cache any more?
##########
iotdb-core/tsfile/src/main/java/org/apache/iotdb/tsfile/read/TsFileDeviceIterator.java:
##########
@@ -74,15 +75,15 @@ public boolean hasNext() {
}
@Override
- public Pair<String, Boolean> next() {
+ public Pair<IDeviceID, Boolean> next() {
if (!hasNext()) {
throw new NoSuchElementException();
}
- Pair<String, long[]> startEndPair = queue.remove();
+ Pair<IDeviceID, long[]> startEndPair = queue.remove();
try {
// get the first measurement node of this device, to know if the device
is aligned
this.measurementNode =
- reader.readMetadataIndexNode(startEndPair.right[0],
startEndPair.right[1]);
+ reader.readMetadataIndexNode(startEndPair.right[0],
startEndPair.right[1], false);
Review Comment:
false or true?
##########
iotdb-core/tsfile/src/main/java/org/apache/iotdb/tsfile/file/metadata/PlainDeviceID.java:
##########
@@ -49,22 +59,52 @@ public int hashCode() {
return deviceID.hashCode();
}
- @Override
public String toString() {
+ if (true) {
+ new RuntimeException().printStackTrace();
+ throw new RuntimeException();
+ }
Review Comment:
```suggestion
```
##########
iotdb-core/tsfile/src/main/java/org/apache/iotdb/tsfile/file/metadata/PlainDeviceID.java:
##########
@@ -29,6 +32,10 @@ public class PlainDeviceID implements IDeviceID {
String deviceID;
public PlainDeviceID(String deviceID) {
+ if (deviceID == null) {
+ new RuntimeException().printStackTrace();
+ throw new RuntimeException();
+ }
Review Comment:
```suggestion
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
