[GitHub] [hadoop] NishthaShah commented on a diff in pull request #5696: HDFS-16946. Fix getTopTokenRealOwners to return String
NishthaShah commented on code in PR #5696:
URL: https://github.com/apache/hadoop/pull/5696#discussion_r1224032808
##
hadoop-hdfs-project/hadoop-hdfs-rbf/src/test/java/org/apache/hadoop/hdfs/server/federation/security/TestRouterSecurityManager.java:
##
@@ -259,6 +272,39 @@ private static String[] getUserGroupForTesting() {
return groupsForTesting;
}
+ @Test
+ public void testGetTopTokenRealOwners() throws Exception {
+// Create conf and start routers with only an RPC service
+Configuration conf = initSecurity();
+
+Configuration routerConf = new RouterConfigBuilder()
+.metrics()
+.rpc()
+.build();
+conf.addResource(routerConf);
+
+Router router = initializeAndStartRouter(conf);
+
+// Create credentials
+UserGroupInformation ugi =
UserGroupInformation.createUserForTesting("router", getUserGroupForTesting());
+Credentials creds = RouterSecurityManager.createCredentials(router, ugi,
"some_renewer");
Review Comment:
@ayushtkn If we remove it, then running testGetTopTokenRealOwners alone is
failing as its not able to find any token
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] NishthaShah commented on a diff in pull request #5696: HDFS-16946. Fix getTopTokenRealOwners to return String
NishthaShah commented on code in PR #5696:
URL: https://github.com/apache/hadoop/pull/5696#discussion_r1222939776
##
hadoop-hdfs-project/hadoop-hdfs-rbf/src/test/java/org/apache/hadoop/hdfs/server/federation/security/TestRouterSecurityManager.java:
##
@@ -81,6 +87,17 @@ public static void createMockSecretManager() throws
IOException {
@Rule
public ExpectedException exceptionRule = ExpectedException.none();
+ private Router initializeAndStartRouter(Configuration configuration) {
+Router router = new Router();
+try {
+ router.init(configuration);
+ router.start();
+} catch (MetricsException e) {
+ //do nothing
Review Comment:
@ayushtkn Are you running via command line and still see failure?
I see consistent successful behaviour when I run full test from cmdline and
via IDE debug/run its passing for me intermittently (3/5 passed for me)
Edit: With the latest commit, eliminated the flaky behaviour
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] NishthaShah commented on a diff in pull request #5696: HDFS-16946. Fix getTopTokenRealOwners to return String
NishthaShah commented on code in PR #5696:
URL: https://github.com/apache/hadoop/pull/5696#discussion_r1222939776
##
hadoop-hdfs-project/hadoop-hdfs-rbf/src/test/java/org/apache/hadoop/hdfs/server/federation/security/TestRouterSecurityManager.java:
##
@@ -81,6 +87,17 @@ public static void createMockSecretManager() throws
IOException {
@Rule
public ExpectedException exceptionRule = ExpectedException.none();
+ private Router initializeAndStartRouter(Configuration configuration) {
+Router router = new Router();
+try {
+ router.init(configuration);
+ router.start();
+} catch (MetricsException e) {
+ //do nothing
Review Comment:
@ayushtkn Are you running via command line and still see failure?
I see consistent successful behaviour when I run full test from cmdline and
via IDE debug/run its passing for me intermittently (3/5 passed for me)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] NishthaShah commented on a diff in pull request #5696: HDFS-16946. Fix getTopTokenRealOwners to return String
NishthaShah commented on code in PR #5696:
URL: https://github.com/apache/hadoop/pull/5696#discussion_r1222881620
##
hadoop-hdfs-project/hadoop-hdfs-rbf/src/test/java/org/apache/hadoop/hdfs/server/federation/security/TestRouterSecurityManager.java:
##
@@ -81,6 +87,17 @@ public static void createMockSecretManager() throws
IOException {
@Rule
public ExpectedException exceptionRule = ExpectedException.none();
+ private Router initializeAndStartRouter(Configuration configuration) {
+Router router = new Router();
+try {
+ router.init(configuration);
+ router.start();
+} catch (MetricsException e) {
+ //do nothing
Review Comment:
Sure I can explore DefaultMetricsSystem.setMiniClusterMode(true);
For me, when I was running via command line, testNotRunningSecretManager
never fail. But yes now when I run/debug via the IDE configurations, it fails
Edit: Via IDE it fails sometimes and not all the 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.
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] NishthaShah commented on a diff in pull request #5696: HDFS-16946. Fix getTopTokenRealOwners to return String
NishthaShah commented on code in PR #5696:
URL: https://github.com/apache/hadoop/pull/5696#discussion_r1222881620
##
hadoop-hdfs-project/hadoop-hdfs-rbf/src/test/java/org/apache/hadoop/hdfs/server/federation/security/TestRouterSecurityManager.java:
##
@@ -81,6 +87,17 @@ public static void createMockSecretManager() throws
IOException {
@Rule
public ExpectedException exceptionRule = ExpectedException.none();
+ private Router initializeAndStartRouter(Configuration configuration) {
+Router router = new Router();
+try {
+ router.init(configuration);
+ router.start();
+} catch (MetricsException e) {
+ //do nothing
Review Comment:
Sure I can explore DefaultMetricsSystem.setMiniClusterMode(true);
For me, when I was running via command line, testNotRunningSecretManager
never fail. But yes now when I run/debug via the IDE configurations, it fails
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] NishthaShah commented on a diff in pull request #5696: HDFS-16946. Fix getTopTokenRealOwners to return String
NishthaShah commented on code in PR #5696:
URL: https://github.com/apache/hadoop/pull/5696#discussion_r1222376005
##
hadoop-hdfs-project/hadoop-hdfs-rbf/src/test/java/org/apache/hadoop/hdfs/server/federation/security/TestRouterSecurityManager.java:
##
@@ -81,6 +87,17 @@ public static void createMockSecretManager() throws
IOException {
@Rule
public ExpectedException exceptionRule = ExpectedException.none();
+ private Router initializeAndStartRouter(Configuration configuration) {
+Router router = new Router();
+try {
+ router.init(configuration);
+ router.start();
+} catch (MetricsException e) {
+ //do nothing
Review Comment:
@goiri Added the log
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] NishthaShah commented on a diff in pull request #5696: HDFS-16946. Fix getTopTokenRealOwners to return String
NishthaShah commented on code in PR #5696:
URL: https://github.com/apache/hadoop/pull/5696#discussion_r1221949081
##
hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/metrics/RBFMetrics.java:
##
@@ -715,8 +716,12 @@ public String getTopTokenRealOwners() {
RouterSecurityManager mgr =
this.router.getRpcServer().getRouterSecurityManager();
if (mgr != null && mgr.getSecretManager() != null) {
- return JSON.toString(mgr.getSecretManager()
- .getTopTokenRealOwners(this.topTokenRealOwners));
+ try {
+return JsonUtil.toJsonString((mgr.getSecretManager()
Review Comment:
@goiri I have pushed the changes successfully from local for the recent
comments, but somehow those are not appearing on this PR
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] NishthaShah commented on a diff in pull request #5696: HDFS-16946. Fix getTopTokenRealOwners to return String
NishthaShah commented on code in PR #5696:
URL: https://github.com/apache/hadoop/pull/5696#discussion_r1219859364
##
hadoop-hdfs-project/hadoop-hdfs-rbf/src/test/java/org/apache/hadoop/hdfs/server/federation/security/TestRouterSecurityManager.java:
##
@@ -259,6 +262,15 @@ private static String[] getUserGroupForTesting() {
return groupsForTesting;
}
+ @Test
+ public void testGetTopTokenRealOwners() throws Exception {
+RouterMBean bean = FederationTestUtils.getBean(
+ROUTER_BEAN, RouterMBean.class);
+String topTokenRealOwners = bean.getTopTokenRealOwners();
+assertNotNull("The value of output should not be null " +
topTokenRealOwners, topTokenRealOwners);
+assertTrue("The value contains incorrect message " + topTokenRealOwners,
!topTokenRealOwners.contains("NameValuePair"));
Review Comment:
Makes sense, I can create some tokens and get it asserted
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] NishthaShah commented on a diff in pull request #5696: HDFS-16946. Fix getTopTokenRealOwners to return String
NishthaShah commented on code in PR #5696:
URL: https://github.com/apache/hadoop/pull/5696#discussion_r1218795727
##
hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/metrics/RBFMetrics.java:
##
@@ -712,13 +715,22 @@ public long getCurrentTokensCount() {
@Override
public String getTopTokenRealOwners() {
Review Comment:
@ayushtkn Moved and tested by adding in TestRouterSecurityManager
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] NishthaShah commented on a diff in pull request #5696: HDFS-16946. Fix getTopTokenRealOwners to return String
NishthaShah commented on code in PR #5696:
URL: https://github.com/apache/hadoop/pull/5696#discussion_r1217519201
##
hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/metrics/RBFMetrics.java:
##
@@ -712,13 +715,22 @@ public long getCurrentTokensCount() {
@Override
public String getTopTokenRealOwners() {
Review Comment:
@ayushtkn Let me know if adding it in TestRouterHDFSContractDelegationToken,
sounds good or we should add it in some more suitable place. (Initially had
tried to add a test in TestRBFMetrics, but this.router.getRpcServer(), is
failing with NullPointerException)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] NishthaShah commented on a diff in pull request #5696: HDFS-16946. Fix getTopTokenRealOwners to return String
NishthaShah commented on code in PR #5696:
URL: https://github.com/apache/hadoop/pull/5696#discussion_r1217512371
##
hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/metrics/RBFMetrics.java:
##
@@ -712,13 +715,22 @@ public long getCurrentTokensCount() {
@Override
public String getTopTokenRealOwners() {
Review Comment:
Sure @ayushtkn, Let me add it in the same class
(TestRouterHDFSContractDelegationToken) where I added to 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.
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] NishthaShah commented on a diff in pull request #5696: HDFS-16946. Fix getTopTokenRealOwners to return String
NishthaShah commented on code in PR #5696: URL: https://github.com/apache/hadoop/pull/5696#discussion_r1209635689 ## hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/metrics/RBFMetrics.java: ## @@ -50,6 +50,8 @@ import javax.management.ObjectName; import javax.management.StandardMBean; +import com.fasterxml.jackson.core.JsonProcessingException; Review Comment: @goiri I had to use fasterxml to fix the main issue - to return a string, I think alternatively we can also use StringBuilder/StringJoiner if using fasterxml is not the desired way here. Two more places where I see similar usage is SlowDiskTracker and SlowPeerTracker -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] NishthaShah commented on a diff in pull request #5696: HDFS-16946. Fix getTopTokenRealOwners to return String
NishthaShah commented on code in PR #5696:
URL: https://github.com/apache/hadoop/pull/5696#discussion_r1209637462
##
hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/metrics/RBFMetrics.java:
##
@@ -712,13 +715,22 @@ public long getCurrentTokensCount() {
@Override
public String getTopTokenRealOwners() {
Review Comment:
We don't have one. I had added
`@Test
public void testGetTopTokenRealOwners() throws Exception {
RouterMBean bean = FederationTestUtils.getBean(
ROUTER_BEAN, RouterMBean.class);
String output = bean.getTopTokenRealOwners();
assertNotNull("The value of output should not be null " + output,
output);
assertTrue("The value contains incorrect message " + output,
!output.contains("NameValuePair"));
}` in TestRouterHDFSContractDelegationToken (A similar function in
RBFMetrics was being called in this test class) to test the patch in local
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] NishthaShah commented on a diff in pull request #5696: HDFS-16946. Fix getTopTokenRealOwners to return String
NishthaShah commented on code in PR #5696: URL: https://github.com/apache/hadoop/pull/5696#discussion_r1209635689 ## hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/metrics/RBFMetrics.java: ## @@ -50,6 +50,8 @@ import javax.management.ObjectName; import javax.management.StandardMBean; +import com.fasterxml.jackson.core.JsonProcessingException; Review Comment: @goiri I had to use fasterxml to fix the main issue - to return a string, I think alternatively we can also use StringBuilder/StringJoiner if using fasterxml is not the desired way 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. To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: common-issues-h...@hadoop.apache.org
