Skip to content

Conversation

@EdColeman
Copy link
Contributor

@EdColeman EdColeman commented Jun 4, 2022

This is more for discussion of #2754, the the fate -print issue. It seems that the code was filtering the transaction.

Comparing the logic in main and in #2215, the logic seems the same, so not sure what is going on.

Also, the original logic seemed correct, but rather than the negative / skip logic that was used, this code flips that. Not sure this is a final solution - but maybe it help points to the issue.

@EdColeman EdColeman requested review from Manno15 and milleruntime June 4, 2022 00:55
@EdColeman EdColeman marked this pull request as draft June 4, 2022 00:55
EdColeman added 3 commits June 4, 2022 01:19
 - Test provided by millerruntime, modified to add cluster accumulo.properties
 - modified logic for fate filters
final String table = getUniqueNames(1)[0];

System.setProperty("accumulo.properties",
"file://" + getCluster().getConfig().getAccumuloPropsFile().getCanonicalPath());
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice!

 - modified filter logic
 - expanded IT test
 - modified unit test, command parsing didn't match IT results
@milleruntime
Copy link
Contributor

I think this could be marked ready for review. There are some white space changes that could be dropped and the logic is confusing but if the test is passing then that is the important part to fix #2754. This PR is definitely closer to being merged than #2780, which is just an updated version of 2215.

@EdColeman EdColeman changed the title modify filter conditions for status Fix Fate print command and improve ShellServerIT test Jun 21, 2022
@EdColeman EdColeman marked this pull request as ready for review June 21, 2022 18:34
@EdColeman EdColeman merged commit 4327bee into apache:main Jun 28, 2022
@EdColeman EdColeman deleted the fate_print_debug branch June 28, 2022 16:09
@EdColeman EdColeman mentioned this pull request Aug 2, 2022
@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.

4 participants