Cassandra-16565 trunk remove Sigar

[Claudenw]

Thanks for sending a pull request! Here are some tips if you're new here:

• Ensure you have added or run the appropriate tests for your PR.
• Be sure to keep the PR description updated to reflect all changes.
• Write your PR title to summarize what this PR proposes.
• If possible, provide a concise example to reproduce the issue for a faster review.
• Read our contributor guidelines
• If you're making a documentation change, see our guide to documentation contribution

Commit messages should follow the following format:

<One sentence description, usually Jira title or CHANGES.txt summary>

<Optional lengthier description (context on patch)>

patch by <Authors>; reviewed by <Reviewers> for CASSANDRA-#####

Co-authored-by: Name1 <email1>
Co-authored-by: Name2 <email2>

The Cassandra Jira

Removed Sigar references from the build scripts and library.
Removed SigarLibrary.java and replaced it with SystemInfo.java and updated the calls.

Switched to OSHI library
Modified warnIfRunningInDegradedMode as noted on the ticket CASSANDRA-16565

[Claudenw]

Should the startup check log a message if it is disabled?

        {
            if (options.isDisabled(getStartupCheckType()))
                return;

            Optional<String> degradations = new SystemInfo().isDegraded();

            if (degradations.isPresent())
                logger.warn("Cassandra server running in degraded mode. " + degradations.get());
            else
                logger.info("Checked OS settings and found them configured for optimal performance.");
        }

[Claudenw]

@smiklosovic I rebased on trunk and picked up the Harry changes. What do we need to do to move this forward?

[Claudenw]

@michaelsembwever short answer is: no, I did not use the script. I have fixed the issues.

As I noted in Slack (and here for public record)

I didn't know that what it was for. I will have to update the 'change dependencies' documentation I recently submitted.

In addition when I tried to run the script on a Linux 6.2.0-39-generic #40~22.04.1-Ubuntu SMP PREEMPT_DYNAMIC Thu Nov 16 10:53:04 UTC 2 x86_64 x86_64 x86_64 GNU/Linux system it failed with 2 errors
sed: can't read s/cassandra\.classpath\.jars\">.*<\/property>/cassandra\.classpath\.jars\">NEW_CLASSPATH<\/property>/: No such file or directory and a much longer one from the next line with the CLASSPATH but same No such file or directory error.

The SED comands in the script have blank files for script names (the single quotes after the -i as noted below:

sed -i '' 's/cassandra\.classpath\.jars\">.*<\/property>/cassandra\.classpath\.jars\">NEW_CLASSPATH<\/property>/' $DIR/project.xml
sed -i '' "s@NEW_CLASSPATH@"$CLASSPATH"@" $DIR/project.xml

If I remove the single quotes it works on my system but I am reticent to include this in this fix.

[smiklosovic]

@Claudenw I was thinking about adding some tests for this. As of now, we just don't test the logic in this method. SystemInfo might be mocked by Mockito on its methods to return whatever we want instead of oshi returning it and then we could make assertions on various corner cases and values so we might watch what degradation string we resolve here.

Another option is to not use Mockito but we might just extend this class and override its methods, returning whatever we want. Then we might do assertions on isDegraded output.

[smiklosovic]

@Claudenw we can use platform method of this class instead of oshi.SystemInfo.getCurrentPlatform() and platform might return PlatformEnum instead of String.

That way, except reusing the code, it will also unblocks us when we will go to mock what this class is doing when testing various scenarios.

[Claudenw]

Switching to returning the PlatformEnum exposes the underlying classes of the SystemInfo class. This will make it more complex to replace the functionality later. I think that SystemInfo should isolate the developer from the underlying classes that provide the functionality. We could make SystemInfo an interface and move the current code to a SystemInfoImpl class, then @jacek-lewandowski suggestion (below) that there be a provider, and your observation that Mockito might be used become easier to implement.

[smiklosovic]

@Claudenw one nice advantage of returning enum is that you can use it in case and it is compile time safe ... We can just compare enums directly. I do not think it is a lot to ask for given how comfortable its usage will be. We are not planning to change the impl anytime soon again.

I can imagine a lot of people would ask "what platform I am at" quite frequently and dealing with "equals" and Linux string etc every time is ... nah.

[smiklosovic]

@Claudenw do you plan to use platform() here?

[Claudenw]

No, it is an internal call, so no need to go though the method.

[smiklosovic]

@Claudenw how do you want to mock the operating system this is run on when testing various scenarios? If we used platform() here then we could mock it returns something else from "Linux".

[jacek-lewandowski]

It looks ok, I have one general comment - I think that something like SystemInfo should have a kind of a provider so that we can easily mock it in tests. Maybe something similar to the clock provider.

[Claudenw]

The latest changes DO NOT implement a provider, and do not expose the underlying SystemInfo implementation.

I still need some clarity on whether or not SystemInfo should be an interface with the current code as an implementation.

[jacek-lewandowski]

@Claudenw I'd say it is up to you because SystemInfo looks like something easy to mock.

[smiklosovic]

@Claudenw I dont think we need an interface for this. It is not like it is a hobby to change the guts of this every quarter or some custom impls would be necessary to justify an interface for that. I think that SystemInfo, as is, is mockable already. cc @jacek-lewandowski

[jacek-lewandowski]

yup, just as said, I'm fine with either approach

[jacek-lewandowski]

@Claudenw will you add a provider for SystemInfo?

[Claudenw]

@michaelsembwever The dev documentation update mentioning the ide/nbproject/update-netbeans-classpaths.sh has been submitted as a pull request to @aratno change for CASSANDRA-17750 (apache/cassandra-website#170). I expect that will be updated shortly and perhaps we can get some focus on it to get it merged.

[Claudenw]

The provider has been added. I think this now implements all agreed suggestions from the reviews.

[jacek-lewandowski]

Please at least memoize the value from the default provider. Also name instance does not suggest a provider - would be good to replace it with get or something like that.