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 2021/03/23 16:03:38 UTC
[GitHub] [ozone] JacksonYao287 opened a new pull request #2079: HDDS-4981. Remove SequenceIdGenerator#StateManagerImpl
JacksonYao287 opened a new pull request #2079:
URL: https://github.com/apache/ozone/pull/2079
## What changes were proposed in this pull request?
Remove SequenceIdGenerator#StateManagerImpl
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-4981
## How was this patch tested?
unit test
--
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: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] JacksonYao287 commented on pull request #2079: HDDS-4981. Remove SequenceIdGenerator#StateManagerImpl
Posted by GitBox <gi...@apache.org>.
JacksonYao287 commented on pull request #2079:
URL: https://github.com/apache/ozone/pull/2079#issuecomment-805789475
> Check `ReconStorageContainerManagerFacade`, in the test, the non ratis id gen can use below `MockSCMHAManager`
>
> ```
> this.scmhaManager = MockSCMHAManager.getInstance(
> true, new SCMDBTransactionBufferImpl());
> ```
the code in MockSCMHAManager is below
```
private MockSCMHAManager(boolean isLeader, DBTransactionBuffer buffer) {
**this.ratisServer = new MockRatisServer();**// it is not depend on 'isLeader'
this.isLeader = isLeader;
this.transactionBuffer = buffer;
}
```
so , after the above method is called , the 'ratisServer' in MockSCMHAManager will not be null;
the code in SequenceIdGenerator is below
```
this.stateManager = new StateManagerHAImpl.Builder()
**.setRatisServer(scmhaManager.getRatisServer())**
.setDBTransactionBuffer(scmhaManager.getDBTransactionBuffer())
.setSequenceIdTable(sequenceIdTable)
.build();
```
```
final SCMHAInvocationHandler invocationHandler
**= new SCMHAInvocationHandler(SEQUENCE_ID, impl, ratisServer);**
```
```
public Object invoke(final Object proxy, final Method method,
final Object[] args) throws Throwable {
try {
long startTime = Time.monotonicNow();
final Object result =
**ratisHandler != null** && method.isAnnotationPresent(Replicate.class) ?
invokeRatis(method, args) :
invokeLocal(method, args);
LOG.debug("Call: {} took {} ms", method, Time.monotonicNow() - startTime);
return result;
} catch(InvocationTargetException iEx) {
throw iEx.getCause();
}
}
```
so , we can see that if the 'ratisHandler' is not null and the method is comment with '@Replicate' , invokeRatis will be called。
--
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: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] JacksonYao287 edited a comment on pull request #2079: HDDS-4981. Remove SequenceIdGenerator#StateManagerImpl
Posted by GitBox <gi...@apache.org>.
JacksonYao287 edited a comment on pull request #2079:
URL: https://github.com/apache/ozone/pull/2079#issuecomment-805789475
> Check `ReconStorageContainerManagerFacade`, in the test, the non ratis id gen can use below `MockSCMHAManager`
>
> ```
> this.scmhaManager = MockSCMHAManager.getInstance(
> true, new SCMDBTransactionBufferImpl());
> ```
@GlenGeng
the code in MockSCMHAManager is below
```
private MockSCMHAManager(boolean isLeader, DBTransactionBuffer buffer) {
this.ratisServer = new MockRatisServer();
this.isLeader = isLeader;
this.transactionBuffer = buffer;
}
```
'this.ratisServer' will always be not null and it is not depend on whether 'isLeader' is true.
so , after the above method is called , the 'ratisServer' in MockSCMHAManager will not be null.
the constructor in SequenceIdGenerator is below
```
this.stateManager = new StateManagerHAImpl.Builder()
.setRatisServer(scmhaManager.getRatisServer())
.setDBTransactionBuffer(scmhaManager.getDBTransactionBuffer())
.setSequenceIdTable(sequenceIdTable)
.build();
```
here，the ratisServer in statemanager will not be true
```
final SCMHAInvocationHandler invocationHandler
= new SCMHAInvocationHandler(SEQUENCE_ID, impl, ratisServer);
```
so，the ratisHandler will not be null
```
public Object invoke(final Object proxy, final Method method,
final Object[] args) throws Throwable {
try {
long startTime = Time.monotonicNow();
final Object result =
ratisHandler != null && method.isAnnotationPresent(Replicate.class) ?
invokeRatis(method, args) :
invokeLocal(method, args);
LOG.debug("Call: {} took {} ms", method, Time.monotonicNow() - startTime);
return result;
} catch(InvocationTargetException iEx) {
throw iEx.getCause();
}
}
```
so , if the 'ratisHandler' is not null and the method is comment with '@Replicate' , 'invokeRatis' will be called。
we can see that whether 'invokeRatis' is called mainly depend on whether 'ratisServer' in SCMHAManager is null.
I modifed MockSCMHAManager so that it can build a instance who's ratisServer is null
does this make sense?
--
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: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] JacksonYao287 edited a comment on pull request #2079: HDDS-4981. Remove SequenceIdGenerator#StateManagerImpl
Posted by GitBox <gi...@apache.org>.
JacksonYao287 edited a comment on pull request #2079:
URL: https://github.com/apache/ozone/pull/2079#issuecomment-805789475
> Check `ReconStorageContainerManagerFacade`, in the test, the non ratis id gen can use below `MockSCMHAManager`
>
> ```
> this.scmhaManager = MockSCMHAManager.getInstance(
> true, new SCMDBTransactionBufferImpl());
> ```
@GlenGeng
the code in MockSCMHAManager is below
```
private MockSCMHAManager(boolean isLeader, DBTransactionBuffer buffer) {
// this.ratisServer will always be not null and it is not depend on whether 'isLeader' is true
this.ratisServer = new MockRatisServer();
this.isLeader = isLeader;
this.transactionBuffer = buffer;
}
```
so , after the above method is called , the 'ratisServer' in MockSCMHAManager will not be null;
the code in SequenceIdGenerator is below
```
this.stateManager = new StateManagerHAImpl.Builder()
// here，the ratisServer in statemanager will not be true
.setRatisServer(scmhaManager.getRatisServer())
.setDBTransactionBuffer(scmhaManager.getDBTransactionBuffer())
.setSequenceIdTable(sequenceIdTable)
.build();
```
```
final SCMHAInvocationHandler invocationHandler
**= new SCMHAInvocationHandler(SEQUENCE_ID, impl, ratisServer);**
```
```
public Object invoke(final Object proxy, final Method method,
final Object[] args) throws Throwable {
try {
long startTime = Time.monotonicNow();
final Object result =
**ratisHandler != null** && method.isAnnotationPresent(Replicate.class) ?
invokeRatis(method, args) :
invokeLocal(method, args);
LOG.debug("Call: {} took {} ms", method, Time.monotonicNow() - startTime);
return result;
} catch(InvocationTargetException iEx) {
throw iEx.getCause();
}
}
```
so , we can see that if the 'ratisHandler' is not null and the method is comment with '@Replicate' , invokeRatis will be called。
--
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: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] JacksonYao287 commented on pull request #2079: HDDS-4981. Remove SequenceIdGenerator#StateManagerImpl
Posted by GitBox <gi...@apache.org>.
JacksonYao287 commented on pull request #2079:
URL: https://github.com/apache/ozone/pull/2079#issuecomment-807976838
> `org.apache.hadoop.ozone.scm.node.TestDecommissionAndMaintenance ` is a known flaky case. You can retry the CI to see if this case can pass or not.
ok, thanks , i wiil retry
--
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: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] JacksonYao287 commented on a change in pull request #2079: HDDS-4981. Remove SequenceIdGenerator#StateManagerImpl
Posted by GitBox <gi...@apache.org>.
JacksonYao287 commented on a change in pull request #2079:
URL: https://github.com/apache/ozone/pull/2079#discussion_r600407420
##########
File path: hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/ha/TestSequenceIDGenerator.java
##########
@@ -70,8 +73,6 @@ public void testSequenceIDGenUponNonRatis() throws Exception {
@Test
public void testSequenceIDGenUponRatis() throws Exception {
OzoneConfiguration conf = SCMTestUtils.getConf();
- // enable ratis based SequenceIDGen
- conf.setBoolean(OZONE_SCM_HA_ENABLE_KEY, true);
Review comment:
before this commit, how a sequenceIdGenerator is build depend on this configuration
```
if (SCMHAUtils.isSCMHAEnabled(conf)) {
this.stateManager = new StateManagerHAImpl.Builder()
.setRatisServer(scmhaManager.getRatisServer())
.setDBTransactionBuffer(scmhaManager.getDBTransactionBuffer())
.setSequenceIdTable(sequenceIdTable)
.build();
} else {
this.stateManager = new StateManagerImpl(sequenceIdTable);
}
```
but now , it is no use here and a sequenceIdGenerator is build depend on scmHAManager。
but we can also keep it here， it does not matter
--
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: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] GlenGeng commented on a change in pull request #2079: HDDS-4981. Remove SequenceIdGenerator#StateManagerImpl
Posted by GitBox <gi...@apache.org>.
GlenGeng commented on a change in pull request #2079:
URL: https://github.com/apache/ozone/pull/2079#discussion_r601020384
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SequenceIdGenerator.java
##########
@@ -91,15 +91,11 @@ public SequenceIdGenerator(ConfigurationSource conf,
this.batchSize = conf.getInt(OZONE_SCM_SEQUENCE_ID_BATCH_SIZE,
OZONE_SCM_SEQUENCE_ID_BATCH_SIZE_DEFAULT);
- if (SCMHAUtils.isSCMHAEnabled(conf)) {
- this.stateManager = new StateManagerHAImpl.Builder()
- .setRatisServer(scmhaManager.getRatisServer())
- .setDBTransactionBuffer(scmhaManager.getDBTransactionBuffer())
- .setSequenceIdTable(sequenceIdTable)
- .build();
- } else {
- this.stateManager = new StateManagerImpl(sequenceIdTable);
- }
+ Preconditions.checkNotNull(scmhaManager);
+ this.stateManager = new StateManagerHAImpl.Builder()
Review comment:
enhance java doc `null if non-Ratis based`
--
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: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] JacksonYao287 commented on a change in pull request #2079: HDDS-4981. Remove SequenceIdGenerator#StateManagerImpl
Posted by GitBox <gi...@apache.org>.
JacksonYao287 commented on a change in pull request #2079:
URL: https://github.com/apache/ozone/pull/2079#discussion_r600407420
##########
File path: hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/ha/TestSequenceIDGenerator.java
##########
@@ -70,8 +73,6 @@ public void testSequenceIDGenUponNonRatis() throws Exception {
@Test
public void testSequenceIDGenUponRatis() throws Exception {
OzoneConfiguration conf = SCMTestUtils.getConf();
- // enable ratis based SequenceIDGen
- conf.setBoolean(OZONE_SCM_HA_ENABLE_KEY, true);
Review comment:
before this commit, how a stateManager of a sequenceIdGenerator is build depend on this configuration
```
if (SCMHAUtils.isSCMHAEnabled(conf)) {
this.stateManager = new StateManagerHAImpl.Builder()
.setRatisServer(scmhaManager.getRatisServer())
.setDBTransactionBuffer(scmhaManager.getDBTransactionBuffer())
.setSequenceIdTable(sequenceIdTable)
.build();
} else {
this.stateManager = new StateManagerImpl(sequenceIdTable);
}
```
but now , it is no use here and a sequenceIdGenerator is build depend on scmHAManager。
but we can also keep this configuration here， it does not matter
--
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: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] JacksonYao287 commented on a change in pull request #2079: HDDS-4981. Remove SequenceIdGenerator#StateManagerImpl
Posted by GitBox <gi...@apache.org>.
JacksonYao287 commented on a change in pull request #2079:
URL: https://github.com/apache/ozone/pull/2079#discussion_r600407420
##########
File path: hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/ha/TestSequenceIDGenerator.java
##########
@@ -70,8 +73,6 @@ public void testSequenceIDGenUponNonRatis() throws Exception {
@Test
public void testSequenceIDGenUponRatis() throws Exception {
OzoneConfiguration conf = SCMTestUtils.getConf();
- // enable ratis based SequenceIDGen
- conf.setBoolean(OZONE_SCM_HA_ENABLE_KEY, true);
Review comment:
before this commit, how a stateManager of a sequenceIdGenerator is build depend on this configuration
```
if (SCMHAUtils.isSCMHAEnabled(conf)) {
this.stateManager = new StateManagerHAImpl.Builder()
.setRatisServer(scmhaManager.getRatisServer())
.setDBTransactionBuffer(scmhaManager.getDBTransactionBuffer())
.setSequenceIdTable(sequenceIdTable)
.build();
} else {
this.stateManager = new StateManagerImpl(sequenceIdTable);
}
```
but now , it is no use here and a sequenceIdGenerator is build totally depend on scmHAManager。
but we can also keep this configuration here， it does not matter
--
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: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] GlenGeng commented on a change in pull request #2079: HDDS-4981. Remove SequenceIdGenerator#StateManagerImpl
Posted by GitBox <gi...@apache.org>.
GlenGeng commented on a change in pull request #2079:
URL: https://github.com/apache/ozone/pull/2079#discussion_r600194555
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SequenceIdGenerator.java
##########
@@ -260,9 +256,13 @@ public Builder setDBTransactionBuffer(DBTransactionBuffer trxBuffer) {
public StateManager build() {
Preconditions.checkNotNull(table);
Preconditions.checkNotNull(buffer);
- Preconditions.checkNotNull(ratisServer);
final StateManager impl = new StateManagerHAImpl(table, buffer);
+
+ if (null == ratisServer) {
Review comment:
no need this logic, just go ahead.
##########
File path: hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/ha/TestSequenceIDGenerator.java
##########
@@ -70,8 +73,6 @@ public void testSequenceIDGenUponNonRatis() throws Exception {
@Test
public void testSequenceIDGenUponRatis() throws Exception {
OzoneConfiguration conf = SCMTestUtils.getConf();
- // enable ratis based SequenceIDGen
- conf.setBoolean(OZONE_SCM_HA_ENABLE_KEY, true);
Review comment:
why remove this configuration ?
##########
File path: hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/ha/TestSequenceIDGenerator.java
##########
@@ -19,23 +19,26 @@
import org.apache.hadoop.hdds.conf.OzoneConfiguration;
import org.apache.hadoop.hdds.scm.metadata.SCMMetadataStore;
import org.apache.hadoop.hdds.scm.metadata.SCMMetadataStoreImpl;
+import org.apache.hadoop.hdds.scm.metadata.SCMDBTransactionBufferImpl;
import org.apache.hadoop.ozone.container.common.SCMTestUtils;
+
import org.junit.Assert;
import org.junit.Test;
-import static org.apache.hadoop.hdds.scm.ScmConfigKeys.OZONE_SCM_HA_ENABLE_KEY;
import static org.apache.hadoop.hdds.scm.ScmConfigKeys.OZONE_SCM_SEQUENCE_ID_BATCH_SIZE;
public class TestSequenceIDGenerator {
@Test
public void testSequenceIDGenUponNonRatis() throws Exception {
OzoneConfiguration conf = SCMTestUtils.getConf();
- conf.setBoolean(OZONE_SCM_HA_ENABLE_KEY, false);
Review comment:
why remove this configuration ?
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/StorageContainerManager.java
##########
@@ -452,6 +452,11 @@ private void initializeSystemManagers(OzoneConfiguration conf,
clusterMap = new NetworkTopologyImpl(conf);
}
+ /*
+ TODO: setSCMHAManager is only called in tests now, so
Review comment:
no need this change.
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/MockSCMHAManager.java
##########
@@ -53,6 +53,10 @@ public static SCMHAManager getInstance(boolean isLeader) {
return new MockSCMHAManager(isLeader);
}
+ public static SCMHAManager getInstance(DBTransactionBuffer buffer) {
Review comment:
no need change this file.
--
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: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] GlenGeng closed pull request #2079: HDDS-4981. Remove SequenceIdGenerator#StateManagerImpl
Posted by GitBox <gi...@apache.org>.
GlenGeng closed pull request #2079:
URL: https://github.com/apache/ozone/pull/2079
--
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: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] JacksonYao287 edited a comment on pull request #2079: HDDS-4981. Remove SequenceIdGenerator#StateManagerImpl
Posted by GitBox <gi...@apache.org>.
JacksonYao287 edited a comment on pull request #2079:
URL: https://github.com/apache/ozone/pull/2079#issuecomment-805789475
> Check `ReconStorageContainerManagerFacade`, in the test, the non ratis id gen can use below `MockSCMHAManager`
>
> ```
> this.scmhaManager = MockSCMHAManager.getInstance(
> true, new SCMDBTransactionBufferImpl());
> ```
@GlenGeng
the code in MockSCMHAManager is below
```
private MockSCMHAManager(boolean isLeader, DBTransactionBuffer buffer) {
this.ratisServer = new MockRatisServer();
this.isLeader = isLeader;
this.transactionBuffer = buffer;
}
```
this.ratisServer will always be not null and it is not depend on whether 'isLeader' is true
so , after the above method is called , the 'ratisServer' in MockSCMHAManager will not be null;
the code in SequenceIdGenerator is below
```
this.stateManager = new StateManagerHAImpl.Builder()
.setRatisServer(scmhaManager.getRatisServer())
.setDBTransactionBuffer(scmhaManager.getDBTransactionBuffer())
.setSequenceIdTable(sequenceIdTable)
.build();
```
here，the ratisServer in statemanager will not be true
```
final SCMHAInvocationHandler invocationHandler
**= new SCMHAInvocationHandler(SEQUENCE_ID, impl, ratisServer);
```
```
public Object invoke(final Object proxy, final Method method,
final Object[] args) throws Throwable {
try {
long startTime = Time.monotonicNow();
final Object result =
ratisHandler != null && method.isAnnotationPresent(Replicate.class) ?
invokeRatis(method, args) :
invokeLocal(method, args);
LOG.debug("Call: {} took {} ms", method, Time.monotonicNow() - startTime);
return result;
} catch(InvocationTargetException iEx) {
throw iEx.getCause();
}
}
```
so , we can see that if the 'ratisHandler' wiil not be null and the method is comment with '@Replicate' , invokeRatis will be called。
--
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: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] JacksonYao287 commented on a change in pull request #2079: HDDS-4981. Remove SequenceIdGenerator#StateManagerImpl
Posted by GitBox <gi...@apache.org>.
JacksonYao287 commented on a change in pull request #2079:
URL: https://github.com/apache/ozone/pull/2079#discussion_r600413168
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/MockSCMHAManager.java
##########
@@ -53,6 +53,10 @@ public static SCMHAManager getInstance(boolean isLeader) {
return new MockSCMHAManager(isLeader);
}
+ public static SCMHAManager getInstance(DBTransactionBuffer buffer) {
Review comment:
MockSCMHAManager.getInstance() will always return a SCMHAManager instance upon ratis, and now i can not build a sequenctIdGenerator upon no ratis by setting 'OZONE_SCM_HA_ENABLE_KEY' to false, so the MockSCMHAManager must have a getInstance method which a return a no-ratis SCMHAManager
--
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: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] JacksonYao287 commented on pull request #2079: HDDS-4981. Remove SequenceIdGenerator#StateManagerImpl
Posted by GitBox <gi...@apache.org>.
JacksonYao287 commented on pull request #2079:
URL: https://github.com/apache/ozone/pull/2079#issuecomment-806785868
> MockRatisServer
> > > Check `ReconStorageContainerManagerFacade`, in the test, the non ratis id gen can use below `MockSCMHAManager`
> > > ```
> > > this.scmhaManager = MockSCMHAManager.getInstance(
> > > true, new SCMDBTransactionBufferImpl());
> > > ```
> >
> >
> > @GlenGeng
> > the constructor of MockSCMHAManager is below
> > ```
> > private MockSCMHAManager(boolean isLeader, DBTransactionBuffer buffer) {
> > this.ratisServer = new MockRatisServer();
> > this.isLeader = isLeader;
> > this.transactionBuffer = buffer;
> > }
> > ```
> >
> >
> > 'this.ratisServer' will always be not null and it is not depend on whether 'isLeader' is true.
> > so , after the above method is called , the 'ratisServer' in MockSCMHAManager will not be null.
> > the stateManager constructor in SequenceIdGenerator is below
> > ```
> > this.stateManager = new StateManagerHAImpl.Builder()
> > .setRatisServer(scmhaManager.getRatisServer())
> > .setDBTransactionBuffer(scmhaManager.getDBTransactionBuffer())
> > .setSequenceIdTable(sequenceIdTable)
> > .build();
> > ```
> >
> >
> > so here，the ‘ratisServer’ in stateManager will not be null and
> > ```
> > final SCMHAInvocationHandler invocationHandler
> > = new SCMHAInvocationHandler(SEQUENCE_ID, impl, ratisServer);
> > ```
> >
> >
> > so，the ratisHandler below will not be null
> > ```
> > public Object invoke(final Object proxy, final Method method,
> > final Object[] args) throws Throwable {
> > try {
> > long startTime = Time.monotonicNow();
> > final Object result =
> > ratisHandler != null && method.isAnnotationPresent(Replicate.class) ?
> > invokeRatis(method, args) :
> > invokeLocal(method, args);
> > LOG.debug("Call: {} took {} ms", method, Time.monotonicNow() - startTime);
> > return result;
> > } catch(InvocationTargetException iEx) {
> > throw iEx.getCause();
> > }
> > }
> > ```
> >
> >
> > so , if the 'ratisHandler' is not null and the method is comment with '@replicate' , 'invokeRatis' will be called。
> > I modifed MockSCMHAManager so that it can build a instance who's ratisServer is null
> > does this make sense?
>
> Hey Jackson, our design decision is, in HA mode, we just `SCMRatisServerImpl`, whose `submitRequest` will call RaftServer; in non-HA mode, or the bypass ratis mode, we use `MockRatisServer`, whose `submitRequest` bypass ratis.
>
> In both situation, the `@Replicate` annotation, `invokeRatis()` and `submitRequest()` take effect like always, yet `MockRatisServer` just transition the call to be a pure in-mem call.
>
> Thus it is fine that the
>
> > the 'ratisHandler' is not null and the method is comment with '@replicate' , 'invokeRatis' will be called
ok，I see ， thanks
--
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: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] JacksonYao287 edited a comment on pull request #2079: HDDS-4981. Remove SequenceIdGenerator#StateManagerImpl
Posted by GitBox <gi...@apache.org>.
JacksonYao287 edited a comment on pull request #2079:
URL: https://github.com/apache/ozone/pull/2079#issuecomment-805789475
> Check `ReconStorageContainerManagerFacade`, in the test, the non ratis id gen can use below `MockSCMHAManager`
>
> ```
> this.scmhaManager = MockSCMHAManager.getInstance(
> true, new SCMDBTransactionBufferImpl());
> ```
@GlenGeng
the code in MockSCMHAManager is below
```
private MockSCMHAManager(boolean isLeader, DBTransactionBuffer buffer) {
this.ratisServer = new MockRatisServer();
this.isLeader = isLeader;
this.transactionBuffer = buffer;
}
```
this.ratisServer will always be not null and it is not depend on whether 'isLeader' is true.
so , after the above method is called , the 'ratisServer' in MockSCMHAManager will not be null.
the code in SequenceIdGenerator is below
```
this.stateManager = new StateManagerHAImpl.Builder()
.setRatisServer(scmhaManager.getRatisServer())
.setDBTransactionBuffer(scmhaManager.getDBTransactionBuffer())
.setSequenceIdTable(sequenceIdTable)
.build();
```
here，the ratisServer in statemanager will not be true
```
final SCMHAInvocationHandler invocationHandler
= new SCMHAInvocationHandler(SEQUENCE_ID, impl, ratisServer);
```
so，the ratisHandler will not be null
```
public Object invoke(final Object proxy, final Method method,
final Object[] args) throws Throwable {
try {
long startTime = Time.monotonicNow();
final Object result =
ratisHandler != null && method.isAnnotationPresent(Replicate.class) ?
invokeRatis(method, args) :
invokeLocal(method, args);
LOG.debug("Call: {} took {} ms", method, Time.monotonicNow() - startTime);
return result;
} catch(InvocationTargetException iEx) {
throw iEx.getCause();
}
}
```
so , if the 'ratisHandler' is not null and the method is comment with '@Replicate' , 'invokeRatis' will be called。
we can see that whether 'invokeRatis' is called mainly depend on whether 'ratisServer' in SCMHAManager is null.
I modifed MockSCMHAManager so that it can build a instance who's ratisServer is null
does this make sense?
--
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: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] GlenGeng edited a comment on pull request #2079: HDDS-4981. Remove SequenceIdGenerator#StateManagerImpl
Posted by GitBox <gi...@apache.org>.
GlenGeng edited a comment on pull request #2079:
URL: https://github.com/apache/ozone/pull/2079#issuecomment-806321057
> > Check `ReconStorageContainerManagerFacade`, in the test, the non ratis id gen can use below `MockSCMHAManager`
> > ```
> > this.scmhaManager = MockSCMHAManager.getInstance(
> > true, new SCMDBTransactionBufferImpl());
> > ```
>
> @GlenGeng
> the constructor of MockSCMHAManager is below
>
> ```
> private MockSCMHAManager(boolean isLeader, DBTransactionBuffer buffer) {
> this.ratisServer = new MockRatisServer();
> this.isLeader = isLeader;
> this.transactionBuffer = buffer;
> }
> ```
>
> 'this.ratisServer' will always be not null and it is not depend on whether 'isLeader' is true.
> so , after the above method is called , the 'ratisServer' in MockSCMHAManager will not be null.
>
> the stateManager constructor in SequenceIdGenerator is below
>
> ```
> this.stateManager = new StateManagerHAImpl.Builder()
> .setRatisServer(scmhaManager.getRatisServer())
> .setDBTransactionBuffer(scmhaManager.getDBTransactionBuffer())
> .setSequenceIdTable(sequenceIdTable)
> .build();
> ```
>
> so here，the ‘ratisServer’ in stateManager will not be null and
>
> ```
> final SCMHAInvocationHandler invocationHandler
> = new SCMHAInvocationHandler(SEQUENCE_ID, impl, ratisServer);
> ```
>
> so，the ratisHandler below will not be null
>
> ```
> public Object invoke(final Object proxy, final Method method,
> final Object[] args) throws Throwable {
> try {
> long startTime = Time.monotonicNow();
> final Object result =
> ratisHandler != null && method.isAnnotationPresent(Replicate.class) ?
> invokeRatis(method, args) :
> invokeLocal(method, args);
> LOG.debug("Call: {} took {} ms", method, Time.monotonicNow() - startTime);
> return result;
> } catch(InvocationTargetException iEx) {
> throw iEx.getCause();
> }
> }
> ```
>
> so , if the 'ratisHandler' is not null and the method is comment with '@replicate' , 'invokeRatis' will be called。
> I modifed MockSCMHAManager so that it can build a instance who's ratisServer is null
>
> does this make sense?
Hey Jackson, our design decision is, in HA mode, we just `SCMRatisServerImpl`, whose `submitRequest` will call RaftServer; in non-HA mode, or the bypass ratis mode, we use `MockRatisServer`, whose `submitRequest` bypass ratis.
In both situation, the `@Replicate` annotation, `invokeRatis()` and `submitRequest()` take effect like always, yet `MockRatisServer` just transition the call to be a pure in-mem call.
Thus it is fine that the
> the 'ratisHandler' is not null and the method is comment with '@replicate' , 'invokeRatis' will be called
--
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: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] JacksonYao287 edited a comment on pull request #2079: HDDS-4981. Remove SequenceIdGenerator#StateManagerImpl
Posted by GitBox <gi...@apache.org>.
JacksonYao287 edited a comment on pull request #2079:
URL: https://github.com/apache/ozone/pull/2079#issuecomment-805789475
> Check `ReconStorageContainerManagerFacade`, in the test, the non ratis id gen can use below `MockSCMHAManager`
>
> ```
> this.scmhaManager = MockSCMHAManager.getInstance(
> true, new SCMDBTransactionBufferImpl());
> ```
@GlenGeng
the code in MockSCMHAManager is below
```
private MockSCMHAManager(boolean isLeader, DBTransactionBuffer buffer) {
// this.ratisServer wiil always be not null and it is not depend on whether 'isLeader' is true
this.ratisServer = new MockRatisServer();
this.isLeader = isLeader;
this.transactionBuffer = buffer;
}
```
so , after the above method is called , the 'ratisServer' in MockSCMHAManager will not be null;
the code in SequenceIdGenerator is below
```
this.stateManager = new StateManagerHAImpl.Builder()
**.setRatisServer(scmhaManager.getRatisServer())**
.setDBTransactionBuffer(scmhaManager.getDBTransactionBuffer())
.setSequenceIdTable(sequenceIdTable)
.build();
```
```
final SCMHAInvocationHandler invocationHandler
**= new SCMHAInvocationHandler(SEQUENCE_ID, impl, ratisServer);**
```
```
public Object invoke(final Object proxy, final Method method,
final Object[] args) throws Throwable {
try {
long startTime = Time.monotonicNow();
final Object result =
**ratisHandler != null** && method.isAnnotationPresent(Replicate.class) ?
invokeRatis(method, args) :
invokeLocal(method, args);
LOG.debug("Call: {} took {} ms", method, Time.monotonicNow() - startTime);
return result;
} catch(InvocationTargetException iEx) {
throw iEx.getCause();
}
}
```
so , we can see that if the 'ratisHandler' is not null and the method is comment with '@Replicate' , invokeRatis will be called。
--
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: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] JacksonYao287 commented on a change in pull request #2079: HDDS-4981. Remove SequenceIdGenerator#StateManagerImpl
Posted by GitBox <gi...@apache.org>.
JacksonYao287 commented on a change in pull request #2079:
URL: https://github.com/apache/ozone/pull/2079#discussion_r600413168
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/MockSCMHAManager.java
##########
@@ -53,6 +53,10 @@ public static SCMHAManager getInstance(boolean isLeader) {
return new MockSCMHAManager(isLeader);
}
+ public static SCMHAManager getInstance(DBTransactionBuffer buffer) {
Review comment:
MockSCMHAManager.getInstance() will always return a SCMHAManager instance upon ratis, and now i can not build a sequenctIdGenerator upon no ratis by setting 'OZONE_SCM_HA_ENABLE_KEY' to false, so the MockSCMHAManager must have a getInstance method which can return a no-ratis SCMHAManager
--
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: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] JacksonYao287 edited a comment on pull request #2079: HDDS-4981. Remove SequenceIdGenerator#StateManagerImpl
Posted by GitBox <gi...@apache.org>.
JacksonYao287 edited a comment on pull request #2079:
URL: https://github.com/apache/ozone/pull/2079#issuecomment-805789475
> Check `ReconStorageContainerManagerFacade`, in the test, the non ratis id gen can use below `MockSCMHAManager`
>
> ```
> this.scmhaManager = MockSCMHAManager.getInstance(
> true, new SCMDBTransactionBufferImpl());
> ```
@GlenGeng
the code in MockSCMHAManager is below
```
private MockSCMHAManager(boolean isLeader, DBTransactionBuffer buffer) {
// this.ratisServer will always be not null and it is not depend on whether 'isLeader' is true
this.ratisServer = new MockRatisServer();
this.isLeader = isLeader;
this.transactionBuffer = buffer;
}
```
so , after the above method is called , the 'ratisServer' in MockSCMHAManager will not be null;
the code in SequenceIdGenerator is below
```
this.stateManager = new StateManagerHAImpl.Builder()
.setRatisServer(scmhaManager.getRatisServer())// here，the ratisserver in statemanager will not be true
.setDBTransactionBuffer(scmhaManager.getDBTransactionBuffer())
.setSequenceIdTable(sequenceIdTable)
.build();
```
```
final SCMHAInvocationHandler invocationHandler
**= new SCMHAInvocationHandler(SEQUENCE_ID, impl, ratisServer);**
```
```
public Object invoke(final Object proxy, final Method method,
final Object[] args) throws Throwable {
try {
long startTime = Time.monotonicNow();
final Object result =
**ratisHandler != null** && method.isAnnotationPresent(Replicate.class) ?
invokeRatis(method, args) :
invokeLocal(method, args);
LOG.debug("Call: {} took {} ms", method, Time.monotonicNow() - startTime);
return result;
} catch(InvocationTargetException iEx) {
throw iEx.getCause();
}
}
```
so , we can see that if the 'ratisHandler' is not null and the method is comment with '@Replicate' , invokeRatis will be called。
--
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: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] JacksonYao287 edited a comment on pull request #2079: HDDS-4981. Remove SequenceIdGenerator#StateManagerImpl
Posted by GitBox <gi...@apache.org>.
JacksonYao287 edited a comment on pull request #2079:
URL: https://github.com/apache/ozone/pull/2079#issuecomment-805789475
> Check `ReconStorageContainerManagerFacade`, in the test, the non ratis id gen can use below `MockSCMHAManager`
>
> ```
> this.scmhaManager = MockSCMHAManager.getInstance(
> true, new SCMDBTransactionBufferImpl());
> ```
@GlenGeng
the constructor of MockSCMHAManager is below
```
private MockSCMHAManager(boolean isLeader, DBTransactionBuffer buffer) {
this.ratisServer = new MockRatisServer();
this.isLeader = isLeader;
this.transactionBuffer = buffer;
}
```
'this.ratisServer' will always be not null and it is not depend on whether 'isLeader' is true.
so , after the above method is called , the 'ratisServer' in MockSCMHAManager will not be null.
the stateManager constructor in SequenceIdGenerator is below
```
this.stateManager = new StateManagerHAImpl.Builder()
.setRatisServer(scmhaManager.getRatisServer())
.setDBTransactionBuffer(scmhaManager.getDBTransactionBuffer())
.setSequenceIdTable(sequenceIdTable)
.build();
```
so here，the ‘ratisServer’ in stateManager will not be null and
```
final SCMHAInvocationHandler invocationHandler
= new SCMHAInvocationHandler(SEQUENCE_ID, impl, ratisServer);
```
so，the ratisHandler below will not be null
```
public Object invoke(final Object proxy, final Method method,
final Object[] args) throws Throwable {
try {
long startTime = Time.monotonicNow();
final Object result =
ratisHandler != null && method.isAnnotationPresent(Replicate.class) ?
invokeRatis(method, args) :
invokeLocal(method, args);
LOG.debug("Call: {} took {} ms", method, Time.monotonicNow() - startTime);
return result;
} catch(InvocationTargetException iEx) {
throw iEx.getCause();
}
}
```
so , if the 'ratisHandler' is not null and the method is comment with '@Replicate' , 'invokeRatis' will be called。
we can see that whether 'invokeRatis' is called mainly depend on whether 'ratisServer' in SCMHAManager is null.
I modifed MockSCMHAManager so that it can build a instance who's ratisServer is null
does this make sense?
--
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: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] JacksonYao287 edited a comment on pull request #2079: HDDS-4981. Remove SequenceIdGenerator#StateManagerImpl
Posted by GitBox <gi...@apache.org>.
JacksonYao287 edited a comment on pull request #2079:
URL: https://github.com/apache/ozone/pull/2079#issuecomment-805789475
> Check `ReconStorageContainerManagerFacade`, in the test, the non ratis id gen can use below `MockSCMHAManager`
>
> ```
> this.scmhaManager = MockSCMHAManager.getInstance(
> true, new SCMDBTransactionBufferImpl());
> ```
@GlenGeng
the code in MockSCMHAManager is below
```
private MockSCMHAManager(boolean isLeader, DBTransactionBuffer buffer) {
**this.ratisServer = new MockRatisServer();**// it is not depend on 'isLeader'
this.isLeader = isLeader;
this.transactionBuffer = buffer;
}
```
so , after the above method is called , the 'ratisServer' in MockSCMHAManager will not be null;
the code in SequenceIdGenerator is below
```
this.stateManager = new StateManagerHAImpl.Builder()
**.setRatisServer(scmhaManager.getRatisServer())**
.setDBTransactionBuffer(scmhaManager.getDBTransactionBuffer())
.setSequenceIdTable(sequenceIdTable)
.build();
```
```
final SCMHAInvocationHandler invocationHandler
**= new SCMHAInvocationHandler(SEQUENCE_ID, impl, ratisServer);**
```
```
public Object invoke(final Object proxy, final Method method,
final Object[] args) throws Throwable {
try {
long startTime = Time.monotonicNow();
final Object result =
**ratisHandler != null** && method.isAnnotationPresent(Replicate.class) ?
invokeRatis(method, args) :
invokeLocal(method, args);
LOG.debug("Call: {} took {} ms", method, Time.monotonicNow() - startTime);
return result;
} catch(InvocationTargetException iEx) {
throw iEx.getCause();
}
}
```
so , we can see that if the 'ratisHandler' is not null and the method is comment with '@Replicate' , invokeRatis will be called。
--
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: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] JacksonYao287 commented on a change in pull request #2079: HDDS-4981. Remove SequenceIdGenerator#StateManagerImpl
Posted by GitBox <gi...@apache.org>.
JacksonYao287 commented on a change in pull request #2079:
URL: https://github.com/apache/ozone/pull/2079#discussion_r600407420
##########
File path: hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/ha/TestSequenceIDGenerator.java
##########
@@ -70,8 +73,6 @@ public void testSequenceIDGenUponNonRatis() throws Exception {
@Test
public void testSequenceIDGenUponRatis() throws Exception {
OzoneConfiguration conf = SCMTestUtils.getConf();
- // enable ratis based SequenceIDGen
- conf.setBoolean(OZONE_SCM_HA_ENABLE_KEY, true);
Review comment:
before this commit, a sequenceIdGenerator is build depend on this configuration
`
if (SCMHAUtils.isSCMHAEnabled(conf)) {
this.stateManager = new StateManagerHAImpl.Builder()
.setRatisServer(scmhaManager.getRatisServer())
.setDBTransactionBuffer(scmhaManager.getDBTransactionBuffer())
.setSequenceIdTable(sequenceIdTable)
.build();
} else {
this.stateManager = new StateManagerImpl(sequenceIdTable);
}
`
but now , it is no use here and a sequenceIdGenerator is build depend on scmHAManager。
we can also keep it here， it does not matter
--
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: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] GlenGeng commented on pull request #2079: HDDS-4981. Remove SequenceIdGenerator#StateManagerImpl
Posted by GitBox <gi...@apache.org>.
GlenGeng commented on pull request #2079:
URL: https://github.com/apache/ozone/pull/2079#issuecomment-806321057
> > Check `ReconStorageContainerManagerFacade`, in the test, the non ratis id gen can use below `MockSCMHAManager`
> > ```
> > this.scmhaManager = MockSCMHAManager.getInstance(
> > true, new SCMDBTransactionBufferImpl());
> > ```
>
> @GlenGeng
> the constructor of MockSCMHAManager is below
>
> ```
> private MockSCMHAManager(boolean isLeader, DBTransactionBuffer buffer) {
> this.ratisServer = new MockRatisServer();
> this.isLeader = isLeader;
> this.transactionBuffer = buffer;
> }
> ```
>
> 'this.ratisServer' will always be not null and it is not depend on whether 'isLeader' is true.
> so , after the above method is called , the 'ratisServer' in MockSCMHAManager will not be null.
>
> the stateManager constructor in SequenceIdGenerator is below
>
> ```
> this.stateManager = new StateManagerHAImpl.Builder()
> .setRatisServer(scmhaManager.getRatisServer())
> .setDBTransactionBuffer(scmhaManager.getDBTransactionBuffer())
> .setSequenceIdTable(sequenceIdTable)
> .build();
> ```
>
> so here，the ‘ratisServer’ in stateManager will not be null and
>
> ```
> final SCMHAInvocationHandler invocationHandler
> = new SCMHAInvocationHandler(SEQUENCE_ID, impl, ratisServer);
> ```
>
> so，the ratisHandler below will not be null
>
> ```
> public Object invoke(final Object proxy, final Method method,
> final Object[] args) throws Throwable {
> try {
> long startTime = Time.monotonicNow();
> final Object result =
> ratisHandler != null && method.isAnnotationPresent(Replicate.class) ?
> invokeRatis(method, args) :
> invokeLocal(method, args);
> LOG.debug("Call: {} took {} ms", method, Time.monotonicNow() - startTime);
> return result;
> } catch(InvocationTargetException iEx) {
> throw iEx.getCause();
> }
> }
> ```
>
> so , if the 'ratisHandler' is not null and the method is comment with '@replicate' , 'invokeRatis' will be called。
> I modifed MockSCMHAManager so that it can build a instance who's ratisServer is null
>
> does this make sense?
Hey Jackson, our design decision is, in HA mode, we just `SCMRatisServerImpl`, whose `submitRequest` will call RaftServer; in non-HA mode, or the bypass ratis mode, we use `MockRatisServer`, whose `submitRequest` bypass ratis.
Thus it is fine that the
> the 'ratisHandler' is not null and the method is comment with '@replicate' , 'invokeRatis' will be called
--
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: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] GlenGeng closed pull request #2079: HDDS-4981. Remove SequenceIdGenerator#StateManagerImpl
Posted by GitBox <gi...@apache.org>.
GlenGeng closed pull request #2079:
URL: https://github.com/apache/ozone/pull/2079
--
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: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] JacksonYao287 edited a comment on pull request #2079: HDDS-4981. Remove SequenceIdGenerator#StateManagerImpl
Posted by GitBox <gi...@apache.org>.
JacksonYao287 edited a comment on pull request #2079:
URL: https://github.com/apache/ozone/pull/2079#issuecomment-805789475
> Check `ReconStorageContainerManagerFacade`, in the test, the non ratis id gen can use below `MockSCMHAManager`
>
> ```
> this.scmhaManager = MockSCMHAManager.getInstance(
> true, new SCMDBTransactionBufferImpl());
> ```
@GlenGeng
the constructor of MockSCMHAManager is below
```
private MockSCMHAManager(boolean isLeader, DBTransactionBuffer buffer) {
this.ratisServer = new MockRatisServer();
this.isLeader = isLeader;
this.transactionBuffer = buffer;
}
```
'this.ratisServer' will always be not null and it is not depend on whether 'isLeader' is true.
so , after the above method is called , the 'ratisServer' in MockSCMHAManager will not be null.
the stateManager constructor in SequenceIdGenerator is below
```
this.stateManager = new StateManagerHAImpl.Builder()
.setRatisServer(scmhaManager.getRatisServer())
.setDBTransactionBuffer(scmhaManager.getDBTransactionBuffer())
.setSequenceIdTable(sequenceIdTable)
.build();
```
so here，the ‘ratisServer’ in stateManager will not be null and
```
final SCMHAInvocationHandler invocationHandler
= new SCMHAInvocationHandler(SEQUENCE_ID, impl, ratisServer);
```
so，the ratisHandler below will not be null
```
public Object invoke(final Object proxy, final Method method,
final Object[] args) throws Throwable {
try {
long startTime = Time.monotonicNow();
final Object result =
ratisHandler != null && method.isAnnotationPresent(Replicate.class) ?
invokeRatis(method, args) :
invokeLocal(method, args);
LOG.debug("Call: {} took {} ms", method, Time.monotonicNow() - startTime);
return result;
} catch(InvocationTargetException iEx) {
throw iEx.getCause();
}
}
```
so , if the 'ratisHandler' is not null and the method is comment with '@Replicate' , 'invokeRatis' will be called。
I modifed MockSCMHAManager so that it can build a instance who's ratisServer is null
does this make sense?
--
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: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] JacksonYao287 commented on a change in pull request #2079: HDDS-4981. Remove SequenceIdGenerator#StateManagerImpl
Posted by GitBox <gi...@apache.org>.
JacksonYao287 commented on a change in pull request #2079:
URL: https://github.com/apache/ozone/pull/2079#discussion_r600407420
##########
File path: hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/ha/TestSequenceIDGenerator.java
##########
@@ -70,8 +73,6 @@ public void testSequenceIDGenUponNonRatis() throws Exception {
@Test
public void testSequenceIDGenUponRatis() throws Exception {
OzoneConfiguration conf = SCMTestUtils.getConf();
- // enable ratis based SequenceIDGen
- conf.setBoolean(OZONE_SCM_HA_ENABLE_KEY, true);
Review comment:
before this commit, how a sequenceIdGenerator is build depend on this configuration
```
if (SCMHAUtils.isSCMHAEnabled(conf)) {
this.stateManager = new StateManagerHAImpl.Builder()
.setRatisServer(scmhaManager.getRatisServer())
.setDBTransactionBuffer(scmhaManager.getDBTransactionBuffer())
.setSequenceIdTable(sequenceIdTable)
.build();
} else {
this.stateManager = new StateManagerImpl(sequenceIdTable);
}
```
but now , it is no use here and a sequenceIdGenerator is build depend on scmHAManager。
we can also keep it here， it does not matter
--
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: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] JacksonYao287 edited a comment on pull request #2079: HDDS-4981. Remove SequenceIdGenerator#StateManagerImpl
Posted by GitBox <gi...@apache.org>.
JacksonYao287 edited a comment on pull request #2079:
URL: https://github.com/apache/ozone/pull/2079#issuecomment-805789475
> Check `ReconStorageContainerManagerFacade`, in the test, the non ratis id gen can use below `MockSCMHAManager`
>
> ```
> this.scmhaManager = MockSCMHAManager.getInstance(
> true, new SCMDBTransactionBufferImpl());
> ```
@GlenGeng
the constructor of MockSCMHAManager is below
```
private MockSCMHAManager(boolean isLeader, DBTransactionBuffer buffer) {
this.ratisServer = new MockRatisServer();
this.isLeader = isLeader;
this.transactionBuffer = buffer;
}
```
'this.ratisServer' will always be not null and it is not depend on whether 'isLeader' is true.
so , after the above method is called , the 'ratisServer' in MockSCMHAManager will not be null.
the stateManager constructor in SequenceIdGenerator is below
```
this.stateManager = new StateManagerHAImpl.Builder()
.setRatisServer(scmhaManager.getRatisServer())
.setDBTransactionBuffer(scmhaManager.getDBTransactionBuffer())
.setSequenceIdTable(sequenceIdTable)
.build();
```
here，the ‘ratisServer’ in stateManager will not be null
```
final SCMHAInvocationHandler invocationHandler
= new SCMHAInvocationHandler(SEQUENCE_ID, impl, ratisServer);
```
so，the ratisHandler will not be null
```
public Object invoke(final Object proxy, final Method method,
final Object[] args) throws Throwable {
try {
long startTime = Time.monotonicNow();
final Object result =
ratisHandler != null && method.isAnnotationPresent(Replicate.class) ?
invokeRatis(method, args) :
invokeLocal(method, args);
LOG.debug("Call: {} took {} ms", method, Time.monotonicNow() - startTime);
return result;
} catch(InvocationTargetException iEx) {
throw iEx.getCause();
}
}
```
so , if the 'ratisHandler' is not null and the method is comment with '@Replicate' , 'invokeRatis' will be called。
we can see that whether 'invokeRatis' is called mainly depend on whether 'ratisServer' in SCMHAManager is null.
I modifed MockSCMHAManager so that it can build a instance who's ratisServer is null
does this make sense?
--
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: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] JacksonYao287 edited a comment on pull request #2079: HDDS-4981. Remove SequenceIdGenerator#StateManagerImpl
Posted by GitBox <gi...@apache.org>.
JacksonYao287 edited a comment on pull request #2079:
URL: https://github.com/apache/ozone/pull/2079#issuecomment-805789475
> Check `ReconStorageContainerManagerFacade`, in the test, the non ratis id gen can use below `MockSCMHAManager`
>
> ```
> this.scmhaManager = MockSCMHAManager.getInstance(
> true, new SCMDBTransactionBufferImpl());
> ```
@GlenGeng
the code in MockSCMHAManager is below
```
private MockSCMHAManager(boolean isLeader, DBTransactionBuffer buffer) {
// this.ratisServer wiil always be not null and it is not depend on whether 'isLeader' is true
_this.ratisServer = new MockRatisServer();_
this.isLeader = isLeader;
this.transactionBuffer = buffer;
}
```
so , after the above method is called , the 'ratisServer' in MockSCMHAManager will not be null;
the code in SequenceIdGenerator is below
```
this.stateManager = new StateManagerHAImpl.Builder()
**.setRatisServer(scmhaManager.getRatisServer())**
.setDBTransactionBuffer(scmhaManager.getDBTransactionBuffer())
.setSequenceIdTable(sequenceIdTable)
.build();
```
```
final SCMHAInvocationHandler invocationHandler
**= new SCMHAInvocationHandler(SEQUENCE_ID, impl, ratisServer);**
```
```
public Object invoke(final Object proxy, final Method method,
final Object[] args) throws Throwable {
try {
long startTime = Time.monotonicNow();
final Object result =
**ratisHandler != null** && method.isAnnotationPresent(Replicate.class) ?
invokeRatis(method, args) :
invokeLocal(method, args);
LOG.debug("Call: {} took {} ms", method, Time.monotonicNow() - startTime);
return result;
} catch(InvocationTargetException iEx) {
throw iEx.getCause();
}
}
```
so , we can see that if the 'ratisHandler' is not null and the method is comment with '@Replicate' , invokeRatis will be called。
--
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: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] JacksonYao287 commented on a change in pull request #2079: HDDS-4981. Remove SequenceIdGenerator#StateManagerImpl
Posted by GitBox <gi...@apache.org>.
JacksonYao287 commented on a change in pull request #2079:
URL: https://github.com/apache/ozone/pull/2079#discussion_r600413168
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/MockSCMHAManager.java
##########
@@ -53,6 +53,10 @@ public static SCMHAManager getInstance(boolean isLeader) {
return new MockSCMHAManager(isLeader);
}
+ public static SCMHAManager getInstance(DBTransactionBuffer buffer) {
Review comment:
MockSCMHAManager.getInstance() will always return a SCMHAManager instance upon ratis, and now i can not build a sequenceIdGenerator upon no ratis by just setting 'OZONE_SCM_HA_ENABLE_KEY' to false, so the MockSCMHAManager must have a getInstance method which can return a no-ratis SCMHAManager
--
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: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] GlenGeng merged pull request #2079: HDDS-4981. Remove SequenceIdGenerator#StateManagerImpl
Posted by GitBox <gi...@apache.org>.
GlenGeng merged pull request #2079:
URL: https://github.com/apache/ozone/pull/2079
--
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: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] GlenGeng commented on pull request #2079: HDDS-4981. Remove SequenceIdGenerator#StateManagerImpl
Posted by GitBox <gi...@apache.org>.
GlenGeng commented on pull request #2079:
URL: https://github.com/apache/ozone/pull/2079#issuecomment-809855443
+1. Thanks @JacksonYao287 for the contribution. I will merge it.
--
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: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] JacksonYao287 edited a comment on pull request #2079: HDDS-4981. Remove SequenceIdGenerator#StateManagerImpl
Posted by GitBox <gi...@apache.org>.
JacksonYao287 edited a comment on pull request #2079:
URL: https://github.com/apache/ozone/pull/2079#issuecomment-805789475
> Check `ReconStorageContainerManagerFacade`, in the test, the non ratis id gen can use below `MockSCMHAManager`
>
> ```
> this.scmhaManager = MockSCMHAManager.getInstance(
> true, new SCMDBTransactionBufferImpl());
> ```
@GlenGeng
the code in MockSCMHAManager is below
```
private MockSCMHAManager(boolean isLeader, DBTransactionBuffer buffer) {
this.ratisServer = new MockRatisServer();
this.isLeader = isLeader;
this.transactionBuffer = buffer;
}
```
this.ratisServer will always be not null and it is not depend on whether 'isLeader' is true
so , after the above method is called , the 'ratisServer' in MockSCMHAManager will not be null;
the code in SequenceIdGenerator is below
```
this.stateManager = new StateManagerHAImpl.Builder()
.setRatisServer(scmhaManager.getRatisServer())
.setDBTransactionBuffer(scmhaManager.getDBTransactionBuffer())
.setSequenceIdTable(sequenceIdTable)
.build();
```
here，the ratisServer in statemanager will not be true
```
final SCMHAInvocationHandler invocationHandler
= new SCMHAInvocationHandler(SEQUENCE_ID, impl, ratisServer);
```
so，the ratisHandler will not be null
```
public Object invoke(final Object proxy, final Method method,
final Object[] args) throws Throwable {
try {
long startTime = Time.monotonicNow();
final Object result =
ratisHandler != null && method.isAnnotationPresent(Replicate.class) ?
invokeRatis(method, args) :
invokeLocal(method, args);
LOG.debug("Call: {} took {} ms", method, Time.monotonicNow() - startTime);
return result;
} catch(InvocationTargetException iEx) {
throw iEx.getCause();
}
}
```
so , we can see that if the 'ratisHandler' wiil not be null and the method is comment with '@Replicate' , invokeRatis will be called。
--
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: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] JacksonYao287 commented on a change in pull request #2079: HDDS-4981. Remove SequenceIdGenerator#StateManagerImpl
Posted by GitBox <gi...@apache.org>.
JacksonYao287 commented on a change in pull request #2079:
URL: https://github.com/apache/ozone/pull/2079#discussion_r600407420
##########
File path: hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/ha/TestSequenceIDGenerator.java
##########
@@ -70,8 +73,6 @@ public void testSequenceIDGenUponNonRatis() throws Exception {
@Test
public void testSequenceIDGenUponRatis() throws Exception {
OzoneConfiguration conf = SCMTestUtils.getConf();
- // enable ratis based SequenceIDGen
- conf.setBoolean(OZONE_SCM_HA_ENABLE_KEY, true);
Review comment:
before this commit, how a sequenceIdGenerator is build depend on this configuration
```
if (SCMHAUtils.isSCMHAEnabled(conf)) {
this.stateManager = new StateManagerHAImpl.Builder()
.setRatisServer(scmhaManager.getRatisServer())
.setDBTransactionBuffer(scmhaManager.getDBTransactionBuffer())
.setSequenceIdTable(sequenceIdTable)
.build();
} else {
this.stateManager = new StateManagerImpl(sequenceIdTable);
}
```
but now , it is no use here and a sequenceIdGenerator is build depend on scmHAManager。
but we can also keep this configuration here， it does not matter
--
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: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] GlenGeng closed pull request #2079: HDDS-4981. Remove SequenceIdGenerator#StateManagerImpl
Posted by GitBox <gi...@apache.org>.
GlenGeng closed pull request #2079:
URL: https://github.com/apache/ozone/pull/2079
--
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: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] JacksonYao287 edited a comment on pull request #2079: HDDS-4981. Remove SequenceIdGenerator#StateManagerImpl
Posted by GitBox <gi...@apache.org>.
JacksonYao287 edited a comment on pull request #2079:
URL: https://github.com/apache/ozone/pull/2079#issuecomment-805789475
> Check `ReconStorageContainerManagerFacade`, in the test, the non ratis id gen can use below `MockSCMHAManager`
>
> ```
> this.scmhaManager = MockSCMHAManager.getInstance(
> true, new SCMDBTransactionBufferImpl());
> ```
@GlenGeng
the code in MockSCMHAManager is below
```
private MockSCMHAManager(boolean isLeader, DBTransactionBuffer buffer) {
this.ratisServer = new MockRatisServer();
this.isLeader = isLeader;
this.transactionBuffer = buffer;
}
```
this.ratisServer will always be not null and it is not depend on whether 'isLeader' is true
so , after the above method is called , the 'ratisServer' in MockSCMHAManager will not be null;
the code in SequenceIdGenerator is below
```
this.stateManager = new StateManagerHAImpl.Builder()
.setRatisServer(scmhaManager.getRatisServer())
.setDBTransactionBuffer(scmhaManager.getDBTransactionBuffer())
.setSequenceIdTable(sequenceIdTable)
.build();
```
here，the ratisServer in statemanager will not be true
```
final SCMHAInvocationHandler invocationHandler
**= new SCMHAInvocationHandler(SEQUENCE_ID, impl, ratisServer);**
```
```
public Object invoke(final Object proxy, final Method method,
final Object[] args) throws Throwable {
try {
long startTime = Time.monotonicNow();
final Object result =
ratisHandler != null && method.isAnnotationPresent(Replicate.class) ?
invokeRatis(method, args) :
invokeLocal(method, args);
LOG.debug("Call: {} took {} ms", method, Time.monotonicNow() - startTime);
return result;
} catch(InvocationTargetException iEx) {
throw iEx.getCause();
}
}
```
so , we can see that if the 'ratisHandler' wiil not be null and the method is comment with '@Replicate' , invokeRatis will be called。
--
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: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] GlenGeng commented on pull request #2079: HDDS-4981. Remove SequenceIdGenerator#StateManagerImpl
Posted by GitBox <gi...@apache.org>.
GlenGeng commented on pull request #2079:
URL: https://github.com/apache/ozone/pull/2079#issuecomment-807974542
`org.apache.hadoop.ozone.scm.node.TestDecommissionAndMaintenance ` is a known flaky case. You can retry the CI to see if this case can pass or not.
--
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: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] GlenGeng commented on pull request #2079: HDDS-4981. Remove SequenceIdGenerator#StateManagerImpl
Posted by GitBox <gi...@apache.org>.
GlenGeng commented on pull request #2079:
URL: https://github.com/apache/ozone/pull/2079#issuecomment-805540555
Check `ReconStorageContainerManagerFacade`, in the test, the non ratis id gen can use below `MockSCMHAManager`
```
this.scmhaManager = MockSCMHAManager.getInstance(
true, new SCMDBTransactionBufferImpl());
```
--
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: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] JacksonYao287 commented on a change in pull request #2079: HDDS-4981. Remove SequenceIdGenerator#StateManagerImpl
Posted by GitBox <gi...@apache.org>.
JacksonYao287 commented on a change in pull request #2079:
URL: https://github.com/apache/ozone/pull/2079#discussion_r600378672
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SequenceIdGenerator.java
##########
@@ -260,9 +256,13 @@ public Builder setDBTransactionBuffer(DBTransactionBuffer trxBuffer) {
public StateManager build() {
Preconditions.checkNotNull(table);
Preconditions.checkNotNull(buffer);
- Preconditions.checkNotNull(ratisServer);
final StateManager impl = new StateManagerHAImpl(table, buffer);
+
+ if (null == ratisServer) {
Review comment:
yes, just go ahead 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: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] JacksonYao287 edited a comment on pull request #2079: HDDS-4981. Remove SequenceIdGenerator#StateManagerImpl
Posted by GitBox <gi...@apache.org>.
JacksonYao287 edited a comment on pull request #2079:
URL: https://github.com/apache/ozone/pull/2079#issuecomment-805789475
> Check `ReconStorageContainerManagerFacade`, in the test, the non ratis id gen can use below `MockSCMHAManager`
>
> ```
> this.scmhaManager = MockSCMHAManager.getInstance(
> true, new SCMDBTransactionBufferImpl());
> ```
@GlenGeng
the code in MockSCMHAManager is below
```
private MockSCMHAManager(boolean isLeader, DBTransactionBuffer buffer) {
this.ratisServer = new MockRatisServer();
this.isLeader = isLeader;
this.transactionBuffer = buffer;
}
```
'this.ratisServer' will always be not null and it is not depend on whether 'isLeader' is true.
so , after the above method is called , the 'ratisServer' in MockSCMHAManager will not be null.
the code in SequenceIdGenerator is below
```
this.stateManager = new StateManagerHAImpl.Builder()
.setRatisServer(scmhaManager.getRatisServer())
.setDBTransactionBuffer(scmhaManager.getDBTransactionBuffer())
.setSequenceIdTable(sequenceIdTable)
.build();
```
here，the ratisServer in statemanager will not be true
```
final SCMHAInvocationHandler invocationHandler
= new SCMHAInvocationHandler(SEQUENCE_ID, impl, ratisServer);
```
so，the ratisHandler will not be null
```
public Object invoke(final Object proxy, final Method method,
final Object[] args) throws Throwable {
try {
long startTime = Time.monotonicNow();
final Object result =
ratisHandler != null && method.isAnnotationPresent(Replicate.class) ?
invokeRatis(method, args) :
invokeLocal(method, args);
LOG.debug("Call: {} took {} ms", method, Time.monotonicNow() - startTime);
return result;
} catch(InvocationTargetException iEx) {
throw iEx.getCause();
}
}
```
so , if the 'ratisHandler' is not null and the method is comment with '@Replicate' , 'invokeRatis' will be called。
we can see that whether 'invokeRatis' is called mainly depend on whether 'ratisServer' in SCMHAManager is null.
I modifed MockSCMHAManager so that it can build a instance who's ratisServer is null
does this make sense?
--
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: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] GlenGeng closed pull request #2079: HDDS-4981. Remove SequenceIdGenerator#StateManagerImpl
Posted by GitBox <gi...@apache.org>.
GlenGeng closed pull request #2079:
URL: https://github.com/apache/ozone/pull/2079
--
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: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] JacksonYao287 commented on a change in pull request #2079: HDDS-4981. Remove SequenceIdGenerator#StateManagerImpl
Posted by GitBox <gi...@apache.org>.
JacksonYao287 commented on a change in pull request #2079:
URL: https://github.com/apache/ozone/pull/2079#discussion_r600413168
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/MockSCMHAManager.java
##########
@@ -53,6 +53,10 @@ public static SCMHAManager getInstance(boolean isLeader) {
return new MockSCMHAManager(isLeader);
}
+ public static SCMHAManager getInstance(DBTransactionBuffer buffer) {
Review comment:
MockSCMHAManager.getInstance() will always return a SCMHAManager instance upon ratis, and now i can not build a sequenceIdGenerator upon no ratis by just setting 'OZONE_SCM_HA_ENABLE_KEY' to false, so the MockSCMHAManager must have a getInstance method which can return a no-ratis SCMHAManager
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/StorageContainerManager.java
##########
@@ -452,6 +452,11 @@ private void initializeSystemManagers(OzoneConfiguration conf,
clusterMap = new NetworkTopologyImpl(conf);
}
+ /*
+ TODO: setSCMHAManager is only called in tests now, so
Review comment:
ok
--
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: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] JacksonYao287 edited a comment on pull request #2079: HDDS-4981. Remove SequenceIdGenerator#StateManagerImpl
Posted by GitBox <gi...@apache.org>.
JacksonYao287 edited a comment on pull request #2079:
URL: https://github.com/apache/ozone/pull/2079#issuecomment-805789475
> Check `ReconStorageContainerManagerFacade`, in the test, the non ratis id gen can use below `MockSCMHAManager`
>
> ```
> this.scmhaManager = MockSCMHAManager.getInstance(
> true, new SCMDBTransactionBufferImpl());
> ```
@GlenGeng
the constructor of MockSCMHAManager is below
```
private MockSCMHAManager(boolean isLeader, DBTransactionBuffer buffer) {
this.ratisServer = new MockRatisServer();
this.isLeader = isLeader;
this.transactionBuffer = buffer;
}
```
'this.ratisServer' will always be not null and it is not depend on whether 'isLeader' is true.
so , after the above method is called , the 'ratisServer' in MockSCMHAManager will not be null.
the stateManager constructor in SequenceIdGenerator is below
```
this.stateManager = new StateManagerHAImpl.Builder()
.setRatisServer(scmhaManager.getRatisServer())
.setDBTransactionBuffer(scmhaManager.getDBTransactionBuffer())
.setSequenceIdTable(sequenceIdTable)
.build();
```
here，the ratisServer in statemanager will not be true
```
final SCMHAInvocationHandler invocationHandler
= new SCMHAInvocationHandler(SEQUENCE_ID, impl, ratisServer);
```
so，the ratisHandler will not be null
```
public Object invoke(final Object proxy, final Method method,
final Object[] args) throws Throwable {
try {
long startTime = Time.monotonicNow();
final Object result =
ratisHandler != null && method.isAnnotationPresent(Replicate.class) ?
invokeRatis(method, args) :
invokeLocal(method, args);
LOG.debug("Call: {} took {} ms", method, Time.monotonicNow() - startTime);
return result;
} catch(InvocationTargetException iEx) {
throw iEx.getCause();
}
}
```
so , if the 'ratisHandler' is not null and the method is comment with '@Replicate' , 'invokeRatis' will be called。
we can see that whether 'invokeRatis' is called mainly depend on whether 'ratisServer' in SCMHAManager is null.
I modifed MockSCMHAManager so that it can build a instance who's ratisServer is null
does this make sense?
--
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: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] JacksonYao287 commented on a change in pull request #2079: HDDS-4981. Remove SequenceIdGenerator#StateManagerImpl
Posted by GitBox <gi...@apache.org>.
JacksonYao287 commented on a change in pull request #2079:
URL: https://github.com/apache/ozone/pull/2079#discussion_r600407420
##########
File path: hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/ha/TestSequenceIDGenerator.java
##########
@@ -70,8 +73,6 @@ public void testSequenceIDGenUponNonRatis() throws Exception {
@Test
public void testSequenceIDGenUponRatis() throws Exception {
OzoneConfiguration conf = SCMTestUtils.getConf();
- // enable ratis based SequenceIDGen
- conf.setBoolean(OZONE_SCM_HA_ENABLE_KEY, true);
Review comment:
before this commit, a sequenceIdGenerator is build depend on this configuration
```
if (SCMHAUtils.isSCMHAEnabled(conf)) {
this.stateManager = new StateManagerHAImpl.Builder()
.setRatisServer(scmhaManager.getRatisServer())
.setDBTransactionBuffer(scmhaManager.getDBTransactionBuffer())
.setSequenceIdTable(sequenceIdTable)
.build();
} else {
this.stateManager = new StateManagerImpl(sequenceIdTable);
}
```
but now , it is no use here and a sequenceIdGenerator is build depend on scmHAManager。
we can also keep it here， it does not matter
--
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: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] JacksonYao287 closed pull request #2079: HDDS-4981. Remove SequenceIdGenerator#StateManagerImpl
Posted by GitBox <gi...@apache.org>.
JacksonYao287 closed pull request #2079:
URL: https://github.com/apache/ozone/pull/2079
--
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: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] GlenGeng commented on a change in pull request #2079: HDDS-4981. Remove SequenceIdGenerator#StateManagerImpl
Posted by GitBox <gi...@apache.org>.
GlenGeng commented on a change in pull request #2079:
URL: https://github.com/apache/ozone/pull/2079#discussion_r600999896
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/MockSCMHAManager.java
##########
@@ -71,6 +76,11 @@ private MockSCMHAManager(boolean isLeader, DBTransactionBuffer buffer) {
this.transactionBuffer = buffer;
}
+ private MockSCMHAManager(SCMDBTransactionBufferImpl buffer) {
+ this.transactionBuffer = buffer;
Review comment:
The change in `MockSCMHAManager` is not needed.
The bypass ratis scm is based on the `MockRatisServer`.
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SequenceIdGenerator.java
##########
@@ -91,15 +91,11 @@ public SequenceIdGenerator(ConfigurationSource conf,
this.batchSize = conf.getInt(OZONE_SCM_SEQUENCE_ID_BATCH_SIZE,
OZONE_SCM_SEQUENCE_ID_BATCH_SIZE_DEFAULT);
- if (SCMHAUtils.isSCMHAEnabled(conf)) {
- this.stateManager = new StateManagerHAImpl.Builder()
- .setRatisServer(scmhaManager.getRatisServer())
- .setDBTransactionBuffer(scmhaManager.getDBTransactionBuffer())
- .setSequenceIdTable(sequenceIdTable)
- .build();
- } else {
- this.stateManager = new StateManagerImpl(sequenceIdTable);
- }
+ Preconditions.checkNotNull(scmhaManager);
+ this.stateManager = new StateManagerHAImpl.Builder()
+ .setRatisServer(scmhaManager.getRatisServer())
+ .setDBTransactionBuffer(scmhaManager.getDBTransactionBuffer())
+ .setSequenceIdTable(sequenceIdTable).build();
Review comment:
Please help rename `StateManagerHAImpl` to `StateManagerImpl`.
##########
File path: hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/ha/TestSequenceIDGenerator.java
##########
@@ -19,23 +19,26 @@
import org.apache.hadoop.hdds.conf.OzoneConfiguration;
import org.apache.hadoop.hdds.scm.metadata.SCMMetadataStore;
import org.apache.hadoop.hdds.scm.metadata.SCMMetadataStoreImpl;
+import org.apache.hadoop.hdds.scm.metadata.SCMDBTransactionBufferImpl;
import org.apache.hadoop.ozone.container.common.SCMTestUtils;
+
import org.junit.Assert;
import org.junit.Test;
-import static org.apache.hadoop.hdds.scm.ScmConfigKeys.OZONE_SCM_HA_ENABLE_KEY;
import static org.apache.hadoop.hdds.scm.ScmConfigKeys.OZONE_SCM_SEQUENCE_ID_BATCH_SIZE;
public class TestSequenceIDGenerator {
@Test
public void testSequenceIDGenUponNonRatis() throws Exception {
OzoneConfiguration conf = SCMTestUtils.getConf();
- conf.setBoolean(OZONE_SCM_HA_ENABLE_KEY, false);
SCMMetadataStore scmMetadataStore = new SCMMetadataStoreImpl(conf);
scmMetadataStore.start(conf);
+ SCMHAManager scmHAManager = MockSCMHAManager.getInstance(
Review comment:
```
SCMHAManager scmHAManager = MockSCMHAManager.getInstance(true,
new SCMDBTransactionBufferImpl());
```
--
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: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] JacksonYao287 commented on a change in pull request #2079: HDDS-4981. Remove SequenceIdGenerator#StateManagerImpl
Posted by GitBox <gi...@apache.org>.
JacksonYao287 commented on a change in pull request #2079:
URL: https://github.com/apache/ozone/pull/2079#discussion_r600413168
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/MockSCMHAManager.java
##########
@@ -53,6 +53,10 @@ public static SCMHAManager getInstance(boolean isLeader) {
return new MockSCMHAManager(isLeader);
}
+ public static SCMHAManager getInstance(DBTransactionBuffer buffer) {
Review comment:
MockSCMHAManager.getInstance() will always return a SCMHAManager instance upon ratis, and now i can not build a sequenceIdGenerator upon no ratis by setting 'OZONE_SCM_HA_ENABLE_KEY' to false, so the MockSCMHAManager must have a getInstance method which can return a no-ratis SCMHAManager
--
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: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] JacksonYao287 commented on pull request #2079: HDDS-4981. Remove SequenceIdGenerator#StateManagerImpl
Posted by GitBox <gi...@apache.org>.
JacksonYao287 commented on pull request #2079:
URL: https://github.com/apache/ozone/pull/2079#issuecomment-806817130
@GlenGeng
please take a review, thanks
--
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: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org