You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by "keith-turner (via GitHub)" <gi...@apache.org> on 2025/09/20 15:42:01 UTC

[PR] improves RG table prop test [accumulo]

keith-turner opened a new pull request, #5905:
URL: https://github.com/apache/accumulo/pull/5905

   Added checking all resource group properties after attempted set and multiple resource groups to the test that ensures table props can not be set on a RG. Also successfully set a property in the test.  These changes cover bug cases like attempting to set a table prop corrupts existing props, setting a table prop throws an exception but still sets the prop, bugs w/ multiple RGs and table props.


-- 
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: notifications-unsubscribe@accumulo.apache.org

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

Re: [PR] improves RG table prop test [accumulo]

Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #5905:
URL: https://github.com/apache/accumulo/pull/5905#discussion_r2370571813


##########
test/src/main/java/org/apache/accumulo/test/conf/PropStoreConfigIT_SimpleSuite.java:
##########
@@ -743,23 +747,44 @@ public void testTablePropInSystemConfigFails() {
                 + " cannot be set at the system or resource group level."
                 + " Set table properties at the namespace or table level."));
       }
+
+      assertEquals(Map.of(), client.instanceOperations().getSystemProperties());

Review Comment:
   Good point.



-- 
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: notifications-unsubscribe@accumulo.apache.org

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

Re: [PR] improves RG table prop test [accumulo]

Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #5905:
URL: https://github.com/apache/accumulo/pull/5905#discussion_r2370381839


##########
test/src/main/java/org/apache/accumulo/test/conf/PropStoreConfigIT_SimpleSuite.java:
##########
@@ -743,23 +747,44 @@ public void testTablePropInSystemConfigFails() {
                 + " cannot be set at the system or resource group level."
                 + " Set table properties at the namespace or table level."));
       }
+
+      assertEquals(Map.of(), client.instanceOperations().getSystemProperties());

Review Comment:
   Isn't this just `assertTrue(map.isEmpty())`? I don't think this needs `assertEquals`



-- 
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: notifications-unsubscribe@accumulo.apache.org

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

Re: [PR] improves RG table prop test [accumulo]

Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner merged PR #5905:
URL: https://github.com/apache/accumulo/pull/5905


-- 
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: notifications-unsubscribe@accumulo.apache.org

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

Re: [PR] improves RG table prop test [accumulo]

Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #5905:
URL: https://github.com/apache/accumulo/pull/5905#discussion_r2370451353


##########
test/src/main/java/org/apache/accumulo/test/conf/PropStoreConfigIT_SimpleSuite.java:
##########
@@ -743,23 +747,44 @@ public void testTablePropInSystemConfigFails() {
                 + " cannot be set at the system or resource group level."
                 + " Set table properties at the namespace or table level."));
       }
+
+      assertEquals(Map.of(), client.instanceOperations().getSystemProperties());

Review Comment:
   Should it not be empty it will give a better error message where it shows the diffs of the two maps.  Seemed like the shortest code to get a good error message.



-- 
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: notifications-unsubscribe@accumulo.apache.org

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

Re: [PR] improves RG table prop test [accumulo]

Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on PR #5905:
URL: https://github.com/apache/accumulo/pull/5905#issuecomment-3315138548

   Looked for a place where using Pair would be more beneficial based on what I learned and found the following. 
   
   https://github.com/apache/accumulo/blob/043bcba6088030037448047462d72583776084a9/core/src/main/java/org/apache/accumulo/core/util/tables/TableNameUtil.java#L43
   
   This method is used frequently and the code and has a lot of calls to getFirst and getSecond, where the namespace is the first and the tablename is second.  This seems like a place where a record would be really be beneficial.


-- 
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: notifications-unsubscribe@accumulo.apache.org

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