You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by GitBox <gi...@apache.org> on 2020/04/21 18:57:24 UTC
[GitHub] [hadoop-ozone] bharatviswa504 opened a new pull request #855: HDDS-3474. Create transactionInfo Table in OmMetadataManager.
bharatviswa504 opened a new pull request #855:
URL: https://github.com/apache/hadoop-ozone/pull/855
## What changes were proposed in this pull request?
1. introduce a new transaction info table which stores transactionInfo.
Key = TRANSACTIONINFO
value = currentTerm-transactionIndex
2. Add new UT's for this table.
3. Provide utility/helper methods to parse the transaction info table value
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-3474
## How was this patch tested?
Added UT.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] bharatviswa504 commented on a change in pull request #855: HDDS-3474. Create transactionInfo Table in OmMetadataManager.
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on a change in pull request #855:
URL: https://github.com/apache/hadoop-ozone/pull/855#discussion_r426091313
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataManagerImpl.java
##########
@@ -131,6 +136,8 @@
public static final String S3_SECRET_TABLE = "s3SecretTable";
public static final String DELEGATION_TOKEN_TABLE = "dTokenTable";
public static final String PREFIX_TABLE = "prefixTable";
+ public static final String TRANSACTION_INFO_TABLE =
+ "transactionInfoTable";
Review comment:
Done.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] bharatviswa504 commented on pull request #855: HDDS-3474. Create transactionInfo Table in OmMetadataManager.
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on pull request #855:
URL: https://github.com/apache/hadoop-ozone/pull/855#issuecomment-624165031
Hi @elek
Any more comments?
I want to get this in, as this is blocking further jira's progress.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] bharatviswa504 edited a comment on pull request #855: HDDS-3474. Create transactionInfo Table in OmMetadataManager.
Posted by GitBox <gi...@apache.org>.
bharatviswa504 edited a comment on pull request #855:
URL: https://github.com/apache/hadoop-ozone/pull/855#issuecomment-624926581
Thank You @elek for the review.
I have addressed the review comments.
> The other strange thing (where I have no real solution) is to creating one rocksdb table just to store one single value. I am not sure if this is the best option (for example in SCM we used a file to store the counters as far as I remember).
> Do we need a generic settings/ table instead of introducing a new table every time when we need to store something on disk? In that case it would be more reasonable to use <String,String>.
>
I have created ratis log table, because in OM each information/entity is stored in a column family. And also we need to persist this information in DB because we want the operation to be atomic, so that when restart, we find lastAppliedIndex from DB, and start applying the transactions after this transaction so that we know this is the last transaction applied to DB and we don't need any special handling of replay in actual requests. (Right now in actual request logic, we have a code for replay, and it is difficult to maintain and the new requests implemented by developers need to know about this detail, with this the user can implement the actual logic for the request and donot need to worry about replay part)
One link I found on is there any disadvantage on increasing column families, not much I found, but it is discussed we shall use writeBuffer of 64MB, not sure if it has impact on this. But in my view having a table and persist this info will be cleaner.
https://github.com/facebook/rocksdb/issues/2426
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] bharatviswa504 edited a comment on pull request #855: HDDS-3474. Create transactionInfo Table in OmMetadataManager.
Posted by GitBox <gi...@apache.org>.
bharatviswa504 edited a comment on pull request #855:
URL: https://github.com/apache/hadoop-ozone/pull/855#issuecomment-621947648
> > Thanks the answer @bharatviswa504 . Still working on my understanding but one small comments in the mean time:
> > If the table is TransactionInfo -> currentTerm-transactionIndex as you write in the Jira: can we modify `Table<String, String>` to have real types + Codec. Can help the understanding IMHO.
>
> Key - TRANSACTIONINFO (string)
> Value - currentTerm#transactionIndex (Also a string) not sure what is meant by real types here?
>
> As both are strings, so used a String codec which already exists.
The reason for not creating a proto object is for every iteration I need to create a proto object and then it will be converted to bytes when flushed to DB (Anyway this info is not passed via wire, it is used in persisting to DB only). I think this is proto creation is unnecessary and the same can be done with simple string i think this should be fine. If needed for understandability I can update java doc and by reading the OMTransactionInfo class will help. Let me know your thoughts.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] bharatviswa504 commented on pull request #855: HDDS-3474. Create transactionInfo Table in OmMetadataManager.
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on pull request #855:
URL: https://github.com/apache/hadoop-ozone/pull/855#issuecomment-624926581
Thank You @elek for the review.
I have addressed the review comments.
`Do we need a generic settings/ table instead of introducing a new table every time when we need to store something on disk? In that case it would be more reasonable to use <String,String>.`
I have created ratis log table, because in OM each information/entity is stored in a column family. And also we need to persist this information in DB because we want the operation to be atomic, so that when restart, we find lastAppliedIndex from DB, and start applying the transactions after this transaction so that we know this is the last transaction applied to DB and we don't need any special handling of replay in actual requests. (Right now in actual request logic, we have a code for replay, and it is difficult to maintain and the new requests implemented by developers need to know about this detail, with this the user can implement the actual logic for the request and donot need to worry about replay part)
https://github.com/facebook/rocksdb/issues/2426
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] bharatviswa504 commented on a change in pull request #855: HDDS-3474. Create transactionInfo Table in OmMetadataManager.
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on a change in pull request #855:
URL: https://github.com/apache/hadoop-ozone/pull/855#discussion_r416022353
##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneConsts.java
##########
@@ -329,4 +329,8 @@ private OzoneConsts() {
public static final String GDPR_ALGORITHM = "algorithm";
+ // Transaction Info
+ public static final String TRANSACTION_INFO_KEY = "TRANSACTIONINFO";
+ public static final String TRANSACTION_INFO_SPLIT_KEY = "-";
+
Review comment:
Done.
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/ratis/OMTransactionInfo.java
##########
@@ -0,0 +1,69 @@
+/*
+ * 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
+ * <p>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p>
+ * 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.hadoop.ozone.om.ratis;
+
+import com.google.common.base.Preconditions;
+import org.apache.hadoop.ozone.OzoneConsts;
+
+/**
+ * TransactionInfo which is applied to OM DB.
+ */
+public class OMTransactionInfo {
+
+ private long currentTerm;
+ private long transactionIndex;
Review comment:
Done.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] bharatviswa504 commented on a change in pull request #855: HDDS-3474. Create transactionInfo Table in OmMetadataManager.
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on a change in pull request #855:
URL: https://github.com/apache/hadoop-ozone/pull/855#discussion_r416006125
##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneConsts.java
##########
@@ -329,4 +329,8 @@ private OzoneConsts() {
public static final String GDPR_ALGORITHM = "algorithm";
+ // Transaction Info
+ public static final String TRANSACTION_INFO_KEY = "TRANSACTIONINFO";
+ public static final String TRANSACTION_INFO_SPLIT_KEY = "-";
+
Review comment:
Technically we don't need it. As in container DB we have iterators to iterate the keys in container, but there are some keys like BCSID, blockCount which are not blocks, so to avoid this they have a filter to skip these keys during iteration (But the same can be done without # also. Might be there it is done to know that these are system defined keys and a way to distinguish. (This is just my understanding, and followed the same when implementing HDDS-3217). In OM, we have tables for each. But I can use the # prefix.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] bharatviswa504 edited a comment on pull request #855: HDDS-3474. Create transactionInfo Table in OmMetadataManager.
Posted by GitBox <gi...@apache.org>.
bharatviswa504 edited a comment on pull request #855:
URL: https://github.com/apache/hadoop-ozone/pull/855#issuecomment-620121969
> Thanks @bharatviswa504 to work on this. I am trying to understand the problem to help with the code review but still not clear the proposal. I added my comments to the parent JIRA.
>
> Especially as this patch introduced `String`->`String` table.
>
> It's also not clear the performance degradation caused by this problem. Can you please help me to understand?
For the questions posted in jira, replied over there.
The main intention of this change is
1. Avoid additional DB looks up, which impact perf. One such jira. HDDS-3129
2. To not handle replay code in actual request execution which was done in previous jiras. This has caused additional complexity in the code. Once this is implemented, we don't need such code.
3. Optimize the restart time.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] bharatviswa504 edited a comment on pull request #855: HDDS-3474. Create transactionInfo Table in OmMetadataManager.
Posted by GitBox <gi...@apache.org>.
bharatviswa504 edited a comment on pull request #855:
URL: https://github.com/apache/hadoop-ozone/pull/855#issuecomment-624165031
Hi @elek
Any more comments? If no more comments by EOD I will commit this. If any more concerns, I can address them in later Jiras.
I want to get this in, as this is blocking further jira's progress.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] arp7 commented on pull request #855: HDDS-3474. Create transactionInfo Table in OmMetadataManager.
Posted by GitBox <gi...@apache.org>.
arp7 commented on pull request #855:
URL: https://github.com/apache/hadoop-ozone/pull/855#issuecomment-620221199
There is also a design note attached to https://issues.apache.org/jira/browse/HDDS-3354.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] bharatviswa504 commented on pull request #855: HDDS-3474. Create transactionInfo Table in OmMetadataManager.
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on pull request #855:
URL: https://github.com/apache/hadoop-ozone/pull/855#issuecomment-620135804
Thank You @hanishakoneru for the review. Addressed review comments.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] bharatviswa504 commented on pull request #855: HDDS-3474. Create transactionInfo Table in OmMetadataManager.
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on pull request #855:
URL: https://github.com/apache/hadoop-ozone/pull/855#issuecomment-621939995
> Thanks the answer @bharatviswa504 . Still working on my understanding but one small comments in the mean time:
>
> If the table is TransactionInfo -> currentTerm-transactionIndex as you write in the Jira: can we modify `Table<String, String>` to have real types + Codec. Can help the understanding IMHO.
Key - TRANSACTIONINFO (string)
Value - currentTerm#transactionIndex (Also a string) not sure what is meant by real types here?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] bharatviswa504 commented on pull request #855: HDDS-3474. Create transactionInfo Table in OmMetadataManager.
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on pull request #855:
URL: https://github.com/apache/hadoop-ozone/pull/855#issuecomment-632831227
Thank You @hanishakoneru for the review and @elek for the discussion and review.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] bharatviswa504 commented on pull request #855: HDDS-3474. Create transactionInfo Table in OmMetadataManager.
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on pull request #855:
URL: https://github.com/apache/hadoop-ozone/pull/855#issuecomment-621947648
> > Thanks the answer @bharatviswa504 . Still working on my understanding but one small comments in the mean time:
> > If the table is TransactionInfo -> currentTerm-transactionIndex as you write in the Jira: can we modify `Table<String, String>` to have real types + Codec. Can help the understanding IMHO.
>
> Key - TRANSACTIONINFO (string)
> Value - currentTerm#transactionIndex (Also a string) not sure what is meant by real types here?
>
> As both are strings, so used a String codec which already exists.
The reason for not creating a proto object is for every iteration I need to create a proto object and then it will be converted to bytes when flushed tooDB. I think this is proto creation is unnecessary and the same can be done with simple string i think this should be fine. If needed for understandability I can update java doc and by reading the OMTransactionInfo class will help. Let me know your thoughts.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] bharatviswa504 commented on pull request #855: HDDS-3474. Create transactionInfo Table in OmMetadataManager.
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on pull request #855:
URL: https://github.com/apache/hadoop-ozone/pull/855#issuecomment-620121969
> Thanks @bharatviswa504 to work on this. I am trying to understand the problem to help with the code review but still not clear the proposal. I added my comments to the parent JIRA.
>
> Especially as this patch introduced `String`->`String` table.
>
> It's also not clear the performance degradation caused by this problem. Can you please help me to understand?
For the questions posted in jira, replied over there.
The main intention of this Jira is
1. Avoid additional DB looks up, which impact perf. One such jira. HDDS-3129
2. To not handle replay code in actual request execution which was done in previous jiras. This has caused additional complexity in the code.
3. Optimize restart time.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] hanishakoneru commented on a change in pull request #855: HDDS-3474. Create transactionInfo Table in OmMetadataManager.
Posted by GitBox <gi...@apache.org>.
hanishakoneru commented on a change in pull request #855:
URL: https://github.com/apache/hadoop-ozone/pull/855#discussion_r426072963
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataManagerImpl.java
##########
@@ -131,6 +136,8 @@
public static final String S3_SECRET_TABLE = "s3SecretTable";
public static final String DELEGATION_TOKEN_TABLE = "dTokenTable";
public static final String PREFIX_TABLE = "prefixTable";
+ public static final String TRANSACTION_INFO_TABLE =
+ "transactionInfoTable";
Review comment:
Can we rename RatisLogTable to TransactionInfoTable to avoid confusion.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] bharatviswa504 edited a comment on pull request #855: HDDS-3474. Create transactionInfo Table in OmMetadataManager.
Posted by GitBox <gi...@apache.org>.
bharatviswa504 edited a comment on pull request #855:
URL: https://github.com/apache/hadoop-ozone/pull/855#issuecomment-621939995
> Thanks the answer @bharatviswa504 . Still working on my understanding but one small comments in the mean time:
>
> If the table is TransactionInfo -> currentTerm-transactionIndex as you write in the Jira: can we modify `Table<String, String>` to have real types + Codec. Can help the understanding IMHO.
Key - TRANSACTIONINFO (string)
Value - currentTerm#transactionIndex (Also a string) not sure what is meant by real types here?
As both are strings, so used a String codec which already exists.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] bharatviswa504 edited a comment on pull request #855: HDDS-3474. Create transactionInfo Table in OmMetadataManager.
Posted by GitBox <gi...@apache.org>.
bharatviswa504 edited a comment on pull request #855:
URL: https://github.com/apache/hadoop-ozone/pull/855#issuecomment-620121969
> Thanks @bharatviswa504 to work on this. I am trying to understand the problem to help with the code review but still not clear the proposal. I added my comments to the parent JIRA.
>
> Especially as this patch introduced `String`->`String` table.
>
> It's also not clear the performance degradation caused by this problem. Can you please help me to understand?
For the questions posted in jira, replied over there.
The main intention of this Jira is
1. Avoid additional DB looks up, which impact perf. One such jira. HDDS-3129
2. To not handle replay code in actual request execution which was done in previous jiras. This has caused additional complexity in the code. Once this is implemented, we don't need such code.
3. Optimize the restart time.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] elek commented on pull request #855: HDDS-3474. Create transactionInfo Table in OmMetadataManager.
Posted by GitBox <gi...@apache.org>.
elek commented on pull request #855:
URL: https://github.com/apache/hadoop-ozone/pull/855#issuecomment-621726993
Thanks the answer @bharatviswa504 . Still working on my understanding but one small comments in the mean time:
If the table is TransactionInfo -> currentTerm-transactionIndex as you write in the Jira: can we modify `Table<String, String>` to have real types + Codec. Can help the understanding IMHO.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] elek commented on pull request #855: HDDS-3474. Create transactionInfo Table in OmMetadataManager.
Posted by GitBox <gi...@apache.org>.
elek commented on pull request #855:
URL: https://github.com/apache/hadoop-ozone/pull/855#issuecomment-618870672
Thanks @bharatviswa504 to work on this. I am trying to understand the problem to help with the code review but still not clear the proposal. I added my comments to the parent JIRA.
Especially as this patch introduced `String`->`String` table.
It's also not clear the performance degradation caused by this problem. Can you please help me to understand?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] hanishakoneru commented on a change in pull request #855: HDDS-3474. Create transactionInfo Table in OmMetadataManager.
Posted by GitBox <gi...@apache.org>.
hanishakoneru commented on a change in pull request #855:
URL: https://github.com/apache/hadoop-ozone/pull/855#discussion_r414128556
##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneConsts.java
##########
@@ -329,4 +329,8 @@ private OzoneConsts() {
public static final String GDPR_ALGORITHM = "algorithm";
+ // Transaction Info
+ public static final String TRANSACTION_INFO_KEY = "TRANSACTIONINFO";
+ public static final String TRANSACTION_INFO_SPLIT_KEY = "-";
+
Review comment:
Question out of curiosity: In another patch (HDDS-3217), the Keys in DB are suffixed with a #. Is that a just convention followed for ContainerDBs or has any significance?
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/ratis/OMTransactionInfo.java
##########
@@ -0,0 +1,69 @@
+/*
+ * 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
+ * <p>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p>
+ * 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.hadoop.ozone.om.ratis;
+
+import com.google.common.base.Preconditions;
+import org.apache.hadoop.ozone.OzoneConsts;
+
+/**
+ * TransactionInfo which is applied to OM DB.
+ */
+public class OMTransactionInfo {
+
+ private long currentTerm;
+ private long transactionIndex;
Review comment:
Can we add a comment here that the transactionIndex corresponds to the Ratis Log Index. Would help if someone is trying to correlate this to Ratis snapshots.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] bharatviswa504 merged pull request #855: HDDS-3474. Create transactionInfo Table in OmMetadataManager.
Posted by GitBox <gi...@apache.org>.
bharatviswa504 merged pull request #855:
URL: https://github.com/apache/hadoop-ozone/pull/855
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] bharatviswa504 edited a comment on pull request #855: HDDS-3474. Create transactionInfo Table in OmMetadataManager.
Posted by GitBox <gi...@apache.org>.
bharatviswa504 edited a comment on pull request #855:
URL: https://github.com/apache/hadoop-ozone/pull/855#issuecomment-621947648
> > Thanks the answer @bharatviswa504 . Still working on my understanding but one small comments in the mean time:
> > If the table is TransactionInfo -> currentTerm-transactionIndex as you write in the Jira: can we modify `Table<String, String>` to have real types + Codec. Can help the understanding IMHO.
Key - TRANSACTIONINFO (string)
Value - currentTerm#transactionIndex (Also a string) not sure what is meant by real types here?
As both are strings, so used a String codec which already exists.
The reason for not creating a proto object is for every iteration I need to create a proto object and then it will be converted to bytes when flushed to DB (Anyway this info is not passed via wire, it is used in persisting to DB only). I think this is proto creation is unnecessary and the same can be done with simple string i think this should be fine. If needed for understandability I can update java doc and by reading the OMTransactionInfo class will help. Let me know your thoughts.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] bharatviswa504 edited a comment on pull request #855: HDDS-3474. Create transactionInfo Table in OmMetadataManager.
Posted by GitBox <gi...@apache.org>.
bharatviswa504 edited a comment on pull request #855:
URL: https://github.com/apache/hadoop-ozone/pull/855#issuecomment-624926581
Thank You @elek for the review.
I have addressed the review comments.
`Do we need a generic settings/ table instead of introducing a new table every time when we need to store something on disk? In that case it would be more reasonable to use <String,String>.`
I have created ratis log table, because in OM each information/entity is stored in a column family. And also we need to persist this information in DB because we want the operation to be atomic, so that when restart, we find lastAppliedIndex from DB, and start applying the transactions after this transaction so that we know this is the last transaction applied to DB and we don't need any special handling of replay in actual requests. (Right now in actual request logic, we have a code for replay, and it is difficult to maintain and the new requests implemented by developers need to know about this detail, with this the user can implement the actual logic for the request and donot need to worry about replay part)
One link I found on is there any disadvantage on increasing column families, not much I found, but it is discussed we shall use writeBuffer of 64MB, not sure if it has impact on this. But in my view having a table and persist this info will be cleaner.
https://github.com/facebook/rocksdb/issues/2426
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org