jt2594838 commented on code in PR #14710:
URL: https://github.com/apache/iotdb/pull/14710#discussion_r1982655346
##########
iotdb-core/node-commons/src/main/java/org/apache/iotdb/commons/memory/MemoryManager.java:
##########
@@ -294,35 +299,35 @@ public synchronized void
releaseWithOutNotify(IMemoryBlock block) {
*
* @param name the name of memory manager
* @param sizeInBytes the total memory size in bytes of memory manager
- * @param enable whether memory management is enabled
+ * @param enabled whether memory management is enabled
* @return the memory manager
*/
public synchronized MemoryManager getOrCreateMemoryManager(
- String name, long sizeInBytes, boolean enable) {
- if (sizeInBytes <= 0) {
- LOGGER.warn("getOrCreateMemoryManager {}: sizeInBytes should be
positive", name);
- }
- if (this.enable
- && sizeInBytes + this.allocatedMemorySizeInBytes >
this.totalMemorySizeInBytes) {
- LOGGER.warn(
- "getOrCreateMemoryManager failed: total memory size {} bytes is less
than allocated memory size {} bytes",
- sizeInBytes,
- allocatedMemorySizeInBytes);
- return null;
- }
+ String name, long sizeInBytes, boolean enabled) {
return children.compute(
name,
(managerName, manager) -> {
- if (manager != null) {
+ if (sizeInBytes <= 0) {
+ LOGGER.warn("getOrCreateMemoryManager {}: sizeInBytes should be
positive", name);
Review Comment:
Return null here？
##########
iotdb-core/node-commons/src/main/java/org/apache/iotdb/commons/memory/MemoryManager.java:
##########
@@ -294,35 +299,35 @@ public synchronized void
releaseWithOutNotify(IMemoryBlock block) {
*
* @param name the name of memory manager
* @param sizeInBytes the total memory size in bytes of memory manager
- * @param enable whether memory management is enabled
+ * @param enabled whether memory management is enabled
* @return the memory manager
*/
public synchronized MemoryManager getOrCreateMemoryManager(
Review Comment:
The behavior of this method is ambiguous.
If the manager does not exist, it is okay.
However, what if the manager already exists and the method argument
conflicts with the manager?
For example, if the existing manager has a size of 10MB, but `sizeInBytes`
is 5MB, or the manager is enabled but the parameter `enabled` is false, what
will happen?
##########
iotdb-core/node-commons/src/main/java/org/apache/iotdb/commons/memory/MemoryManager.java:
##########
@@ -294,35 +299,35 @@ public synchronized void
releaseWithOutNotify(IMemoryBlock block) {
*
* @param name the name of memory manager
* @param sizeInBytes the total memory size in bytes of memory manager
- * @param enable whether memory management is enabled
+ * @param enabled whether memory management is enabled
* @return the memory manager
*/
public synchronized MemoryManager getOrCreateMemoryManager(
- String name, long sizeInBytes, boolean enable) {
- if (sizeInBytes <= 0) {
- LOGGER.warn("getOrCreateMemoryManager {}: sizeInBytes should be
positive", name);
- }
- if (this.enable
- && sizeInBytes + this.allocatedMemorySizeInBytes >
this.totalMemorySizeInBytes) {
- LOGGER.warn(
- "getOrCreateMemoryManager failed: total memory size {} bytes is less
than allocated memory size {} bytes",
- sizeInBytes,
- allocatedMemorySizeInBytes);
- return null;
- }
+ String name, long sizeInBytes, boolean enabled) {
return children.compute(
name,
(managerName, manager) -> {
- if (manager != null) {
+ if (sizeInBytes <= 0) {
+ LOGGER.warn("getOrCreateMemoryManager {}: sizeInBytes should be
positive", name);
+ }
+ if (this.enabled
+ && sizeInBytes + this.allocatedMemorySizeInBytes >
this.totalMemorySizeInBytes) {
LOGGER.warn(
- "getOrCreateMemoryManager failed: memory manager {} already
exists, it's size is {}, enable is {}",
+ "getOrCreateMemoryManager failed: total memory size {} bytes
is less than allocated memory size {} bytes",
+ sizeInBytes,
+ allocatedMemorySizeInBytes);
+ return null;
+ }
+ if (manager != null) {
+ LOGGER.debug(
+ "getOrCreateMemoryManager failed: memory manager {} already
exists, it's size is {}, enabled is {}",
Review Comment:
Why do you call it "failed"?
##########
iotdb-core/node-commons/src/main/java/org/apache/iotdb/commons/memory/MemoryManager.java:
##########
@@ -294,35 +299,35 @@ public synchronized void
releaseWithOutNotify(IMemoryBlock block) {
*
* @param name the name of memory manager
* @param sizeInBytes the total memory size in bytes of memory manager
- * @param enable whether memory management is enabled
+ * @param enabled whether memory management is enabled
* @return the memory manager
*/
public synchronized MemoryManager getOrCreateMemoryManager(
- String name, long sizeInBytes, boolean enable) {
- if (sizeInBytes <= 0) {
- LOGGER.warn("getOrCreateMemoryManager {}: sizeInBytes should be
positive", name);
- }
- if (this.enable
- && sizeInBytes + this.allocatedMemorySizeInBytes >
this.totalMemorySizeInBytes) {
- LOGGER.warn(
- "getOrCreateMemoryManager failed: total memory size {} bytes is less
than allocated memory size {} bytes",
- sizeInBytes,
- allocatedMemorySizeInBytes);
- return null;
- }
+ String name, long sizeInBytes, boolean enabled) {
return children.compute(
name,
(managerName, manager) -> {
- if (manager != null) {
+ if (sizeInBytes <= 0) {
+ LOGGER.warn("getOrCreateMemoryManager {}: sizeInBytes should be
positive", name);
+ }
+ if (this.enabled
+ && sizeInBytes + this.allocatedMemorySizeInBytes >
this.totalMemorySizeInBytes) {
LOGGER.warn(
- "getOrCreateMemoryManager failed: memory manager {} already
exists, it's size is {}, enable is {}",
+ "getOrCreateMemoryManager failed: total memory size {} bytes
is less than allocated memory size {} bytes",
+ sizeInBytes,
+ allocatedMemorySizeInBytes);
+ return null;
+ }
Review Comment:
If the manager already exists, is the check correct?
--
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]
