Skip to content

Conversation

@foster33
Copy link
Contributor

@foster33 foster33 commented Jun 8, 2021

Fixes #2149

Fixes the bug where the config command inside the accumulo shell returns the properties unordered by using ImmutableSortedMap.copyOf.

There is only one instance that the shell output is directly effected by this so the other TreeMaps that were replaced I left alone and did not want to import additional things if it was not needed.

If there are other spots in the commit 4ea9bf1 that would directly benefit from the ImmutableSortedMap.copyOf change, I can make those changes as well.

@foster33
Copy link
Contributor Author

foster33 commented Jun 8, 2021

I noticed that there were two parts in the previously linked commit, in Admin.java where TreeMaps were replaced in printNameSpaceConfiguration and printTableConfiguration . I am not sure where these would be printed but maybe these would benefit from a change.

@DomGarguilo
Copy link
Member

Looks like the change from 4ea9bf1 in TableOperationsHelperTest.check() may benefit from being changed back to treemap since there is a comparison to a treemap.

@Manno15
Copy link
Contributor

Manno15 commented Jun 8, 2021

From my testing, Admin.java is fine as is. I tested both with and without the change and received the same results from the dumpConfig command.

@EdColeman
Copy link
Contributor

@Manno15 - can you explain why the results would be the same? Looking at the dump configuration command implementation there does not seem to be any sorting - could it be just an accident that they matched and with either different values or values created in a different order that they would differ? It seems safest to assume that where a TreeMap was used there might be underlying code somewhere that was built using that assumption - unless it can be determined that is not the case.

@Manno15
Copy link
Contributor

Manno15 commented Jun 8, 2021

@EdColeman I figured out where my confusion came from. I should have paid closer attention, the printTableConfiguration checks first to make sure it is a table property. Therefore, the entire file was correct in that everything was ordered by table (the format for dump config is different than the regular config command. It follows the format config -t accumulo.metadata -s table.split.threshold=64M). Everything after the -s wasn't in sorted order as it was in older versions.

@foster33 So yes, both of those print functions should have the same change as the ConfigCommand, my mistake on not catching that earlier.

@ctubbsii
Copy link
Member

ctubbsii commented Jun 8, 2021

There were a few places where #2122 changed the TreeMap to a non-sorted Map:

  1. NamespaceOperationsHelper - this is only used for looping to update another data structure, and the order doesn't matter, so this is fine
  2. TableOperationsHelper - same as NamespaceOperationsHelper; order doesn't matter, so this is fine
  3. TableOperationsHelperTest - does not matter, because the map equality assertion it is checking will succeed whether or not the map is sorted (it only matters that the maps contain the same mapping, regardless of order)
  4. Admin class - this one matters for presentation; both printTableConfiguration and printNamespaceConfiguration should use the ImmutableSortedMap.copyOf, to ensure the order is preserved
  5. ConfigCommand - this one matters for the shell

Copy link
Member

@ctubbsii ctubbsii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, but the TableOperationsHelperTest change isn't necessary, and it's best to avoid Guava when possible (fewer uses of Guava means fewer potential conflicts later).

@ctubbsii ctubbsii merged commit 99ca22d into apache:main Jun 8, 2021
@foster33 foster33 deleted the acc_2149 branch June 8, 2021 17:37
@ctubbsii ctubbsii added this to the 2.1.0 milestone Jul 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Properties from 'config' command return unordered

5 participants