You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@amoro.apache.org by "Jzjsnow (via GitHub)" <gi...@apache.org> on 2025/01/17 02:35:58 UTC

[PR] [AMORO-3329] Add MemorySize for parsing memory value and unit [amoro]

Jzjsnow opened a new pull request, #3409:
URL: https://github.com/apache/amoro/pull/3409

   <!--
   Thanks for sending a pull request!
   
   Here are some tips for you:
     1. If this is your first time, please read our contributor guidelines: https://amoro.apache.org/how-to-contribute/
     2. If the PR is related to an issue in https://github.com/apache/amoro/issues, add '[AMORO-XXXX]' in your PR title, e.g., '[AMORO-XXXX] Your PR title ...'.
     3. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP][AMORO-XXXX] Your PR title ...'.
   -->
   
   ## Why are the changes needed?
   Currently, when we use flink optimizer groups, the memory parameters `flink-conf.jobmanager.memory.process.size` and `flink-conf.taskmanager.memory.process.size` only support plain numeric strings or strings with `m/g` suffix. If we configure them with mb/gb suffix, we'll get wrong parsed results. And the default unit for plain numbers is mb. The default unit for plain numeric strings is `mb`, which is inconsistent with the default unit of byte in the flink native parameter and may cause inconvenience for users familiar with flink when using this parameter.
   
   Close #3329.
   
   ## Brief change log
   <!--
   Clearly describe the changes made in modules, classes, methods, etc.
   -->
   
   - Add a new class `MemorySize` to the `amoro-common` module for parsing flink's memory parameters, consistent with flink's native parsing logic.
   - Update the values in the column `properties` of ams metadata tables `resource_group` and `resource` to automatically add the `MB` suffix to the flink jm and tm memory parameters that have been set and saved as plain numeric strings
   
   ## How was this patch tested?
   
   - [ ] Add some test cases that check the changes thoroughly including negative and positive cases if possible
   
   - [ ] Add screenshots for manual tests if appropriate
   
   - [ ] Run test locally before making a pull request
   
   ## Documentation
   
   - Does this pull request introduce a new feature? (yes / no)
   - If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented)
   


-- 
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: commits-unsubscribe@amoro.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [AMORO-3329] Add MemorySize for parsing memory value and unit [amoro]

Posted by "Jzjsnow (via GitHub)" <gi...@apache.org>.
Jzjsnow commented on code in PR #3409:
URL: https://github.com/apache/amoro/pull/3409#discussion_r1922953546


##########
amoro-common/src/main/java/org/apache/amoro/utils/MemorySize.java:
##########
@@ -0,0 +1,410 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.amoro.utils;
+
+import static org.apache.amoro.shade.guava32.com.google.common.base.Preconditions.checkArgument;
+import static org.apache.amoro.utils.MemorySize.MemoryUnit.BYTES;
+import static org.apache.amoro.utils.MemorySize.MemoryUnit.GIGA_BYTES;
+import static org.apache.amoro.utils.MemorySize.MemoryUnit.KILO_BYTES;
+import static org.apache.amoro.utils.MemorySize.MemoryUnit.MEGA_BYTES;
+import static org.apache.amoro.utils.MemorySize.MemoryUnit.TERA_BYTES;
+
+import java.math.BigDecimal;
+import java.util.Arrays;
+import java.util.Comparator;
+import java.util.List;
+import java.util.Locale;
+import java.util.Optional;
+import java.util.stream.IntStream;
+
+/**
+ * MemorySize is a representation of a number of bytes, viewable in different units. Copied from
+ * Apache Flink.
+ *
+ * <h2>Parsing</h2>
+ *
+ * <p>The size can be parsed from a text expression. If the expression is a pure number, the value
+ * will be interpreted as bytes.

Review Comment:
   Thanks for the suggestion, I've revised the Java doc in the latest commit to make the original source of the class more clear and explicit.



-- 
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: commits-unsubscribe@amoro.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [AMORO-3329] Add MemorySize for parsing memory value and unit [amoro]

Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #3409:
URL: https://github.com/apache/amoro/pull/3409#issuecomment-2597416163

   ## [Codecov](https://app.codecov.io/gh/apache/amoro/pull/3409?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
   Attention: Patch coverage is `76.59574%` with `33 lines` in your changes missing coverage. Please review.
   > Project coverage is 27.72%. Comparing base [(`243d289`)](https://app.codecov.io/gh/apache/amoro/commit/243d2894c6bbba59b74be97a1ea93f935f0bd9ad?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`9334783`)](https://app.codecov.io/gh/apache/amoro/commit/93347831a4f7ec462a92743e168fcebd1290c77f?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
   > Report is 30 commits behind head on master.
   
   | [Files with missing lines](https://app.codecov.io/gh/apache/amoro/pull/3409?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
   |---|---|---|
   | [...c/main/java/org/apache/amoro/utils/MemorySize.java](https://app.codecov.io/gh/apache/amoro/pull/3409?src=pr&el=tree&filepath=amoro-common%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Famoro%2Futils%2FMemorySize.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-YW1vcm8tY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9hbW9yby91dGlscy9NZW1vcnlTaXplLmphdmE=) | 77.53% | [26 Missing and 5 partials :warning: ](https://app.codecov.io/gh/apache/amoro/pull/3409?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
   | [.../amoro/server/manager/FlinkOptimizerContainer.java](https://app.codecov.io/gh/apache/amoro/pull/3409?src=pr&el=tree&filepath=amoro-ams%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Famoro%2Fserver%2Fmanager%2FFlinkOptimizerContainer.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-YW1vcm8tYW1zL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9hbW9yby9zZXJ2ZXIvbWFuYWdlci9GbGlua09wdGltaXplckNvbnRhaW5lci5qYXZh) | 33.33% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/amoro/pull/3409?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
   
   <details><summary>Additional details and impacted files</summary>
   
   
   ```diff
   @@             Coverage Diff              @@
   ##             master    #3409      +/-   ##
   ============================================
   + Coverage     21.59%   27.72%   +6.13%     
   - Complexity     2309     3566    +1257     
   ============================================
     Files           426      596     +170     
     Lines         39719    48528    +8809     
     Branches       5624     6263     +639     
   ============================================
   + Hits           8577    13456    +4879     
   - Misses        30414    34133    +3719     
   - Partials        728      939     +211     
   ```
   
   | [Flag](https://app.codecov.io/gh/apache/amoro/pull/3409/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
   |---|---|---|
   | [core](https://app.codecov.io/gh/apache/amoro/pull/3409/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `27.72% <76.59%> (?)` | |
   | [trino](https://app.codecov.io/gh/apache/amoro/pull/3409/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   </details>
   
   [:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/amoro/pull/3409?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).   
   :loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
   


-- 
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: commits-unsubscribe@amoro.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [AMORO-3329] Add MemorySize for parsing memory value and unit [amoro]

Posted by "zhoujinsong (via GitHub)" <gi...@apache.org>.
zhoujinsong commented on code in PR #3409:
URL: https://github.com/apache/amoro/pull/3409#discussion_r1921988293


##########
amoro-common/src/main/java/org/apache/amoro/utils/MemorySize.java:
##########
@@ -0,0 +1,410 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.amoro.utils;
+
+import static org.apache.amoro.shade.guava32.com.google.common.base.Preconditions.checkArgument;
+import static org.apache.amoro.utils.MemorySize.MemoryUnit.BYTES;
+import static org.apache.amoro.utils.MemorySize.MemoryUnit.GIGA_BYTES;
+import static org.apache.amoro.utils.MemorySize.MemoryUnit.KILO_BYTES;
+import static org.apache.amoro.utils.MemorySize.MemoryUnit.MEGA_BYTES;
+import static org.apache.amoro.utils.MemorySize.MemoryUnit.TERA_BYTES;
+
+import java.math.BigDecimal;
+import java.util.Arrays;
+import java.util.Comparator;
+import java.util.List;
+import java.util.Locale;
+import java.util.Optional;
+import java.util.stream.IntStream;
+
+/**
+ * MemorySize is a representation of a number of bytes, viewable in different units. Copied from
+ * Apache Flink.
+ *
+ * <h2>Parsing</h2>
+ *
+ * <p>The size can be parsed from a text expression. If the expression is a pure number, the value
+ * will be interpreted as bytes.

Review Comment:
   Could you add some information in the Java doc to tell where this file is copied from?
   As I know, it was copied from the Flink project.



-- 
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: commits-unsubscribe@amoro.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [AMORO-3329] Add MemorySize for parsing memory value and unit [amoro]

Posted by "zhoujinsong (via GitHub)" <gi...@apache.org>.
zhoujinsong merged PR #3409:
URL: https://github.com/apache/amoro/pull/3409


-- 
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: commits-unsubscribe@amoro.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org