Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create a tool for verifying WAL assumptions #123

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

billierinaldi
Copy link
Contributor

Suggestions for additional verifications that could be performed or other improvements are welcome!

billierinaldi and others added 6 commits March 24, 2020 20:44
Made these changes on Github, if they compile I will be shocked.
* Set exit code on failure for scripting

* remove cruft

Co-authored-by: billierinaldi <billie.rinaldi@gmail.com>
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.

FWIW, the build failure in Travis is because this testing framework is for testing non-public API, whereas the expectation is that the accumulo-testing repo be restricted to testing public API. However, I can see the value in providing a testing framework for WAL behavior for different file system types.... so I'm not sure if we should:

  1. relax the restriction,
  2. find a new home for the proposed test,
  3. promote some portion of WAL code to public API (or SPI), or
  4. just work around it (perhaps temporarily) by using fully-qualified class names in the code rather than use import statements.

I haven't reviewed the specific test in detail, but I did look it over and, in general, I don't care if we add new tests to the accumulo-testing package, since nothing in there is public API, we don't do releases of it, and the addition of a new test is not an imposition on anybody using the existing tests. So, the test looks fine to me (other than the build failure).

@keith-turner
Copy link
Contributor

keith-turner commented Mar 27, 2020

Looking at the test its use of internal accumulo config and volume manager are likely susceptible to breakage between accumulo versions as those areas of the code are changing as we pay down tech debt. Ideally the test could only ref the pluggable log closer which is more stable. However not sure if that is possible, will look into it more next week.

@billierinaldi
Copy link
Contributor Author

Thanks for taking a look, @ctubbsii and @keith-turner. That's a good point. One option would be for this to be a standalone tool in Accumulo itself, like the troubleshooting tools. Regarding technical debt, working on this has made me think it would be a lot easier if we had an interface that had all the WAL file system operations in the same place. That is, if the LogCloser also opened the files in addition to closing them (closing meaning lease recovery in this case). That might allow us to wrap a file system that doesn't have leases with our own custom lease handling, if we wanted.

@ctubbsii ctubbsii changed the base branch from master to main August 6, 2020 06:17
@ctubbsii
Copy link
Member

This is quite old. Has anybody looked at this recently and interested in picking it back up?

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.

None yet

3 participants