Skip to content

Pass empty Configuration in DelegatingCluster#8

Merged
nchandrappa merged 1 commit intoyugabyte:3.8.0-yb-xfrom
ebyhr:delegating-cluster-npe
Aug 5, 2021
Merged

Pass empty Configuration in DelegatingCluster#8
nchandrappa merged 1 commit intoyugabyte:3.8.0-yb-xfrom
ebyhr:delegating-cluster-npe

Conversation

@ebyhr
Copy link
Copy Markdown

@ebyhr ebyhr commented Jul 24, 2021

@ebyhr ebyhr mentioned this pull request Jul 24, 2021
3 tasks
@ebyhr
Copy link
Copy Markdown
Author

ebyhr commented Jul 30, 2021

@tedyu @nmalladi Could you review this PR when you have time?

@ebyhr
Copy link
Copy Markdown
Author

ebyhr commented Aug 3, 2021

@tedyu @nmalladi Gentle reminder.

@tedyu
Copy link
Copy Markdown

tedyu commented Aug 4, 2021

Seems reasonable to me.

@nchandrappa
What do you think ?

@nchandrappa
Copy link
Copy Markdown

Hello @ebyhr, Thanks for submitting the PR.

You have mentioned the code fix is to prevent an NPE, is it possible to define a test case scenario when NPE can occur.

Also, DelegatingCluster is an abstract class, and based on the DelegatingClusterIntegrationTest super constructor is not getting invoked. can you please provide the requested details before merging the PR?

Thanks,
Nikhil

@ebyhr
Copy link
Copy Markdown
Author

ebyhr commented Aug 4, 2021

Here is the test case that fails before this change and succeeds after that.

import static org.testng.Assert.assertNotNull;

import org.testng.annotations.Test;

public class DelegateClusterTest {
  @Test(groups = "short")
  public void should_not_throw_null_pointer_exception() {
    Cluster cluster = Cluster.builder().addContactPoints("localhost").build();
    SimpleDelegatingCluster delegatingCluster = new SimpleDelegatingCluster(cluster);
    assertNotNull(delegatingCluster);
  }

  private static class SimpleDelegatingCluster extends DelegatingCluster {
    private final Cluster cluster;

    SimpleDelegatingCluster(Cluster cluster) {
      this.cluster = cluster;
    }

    @Override
    protected Cluster delegate() {
      return cluster;
    }
  }
}

@nchandrappa
Copy link
Copy Markdown

Thanks for providing the testcase @ebyhr. Code fix looks good.

@nchandrappa nchandrappa merged commit 2f71680 into yugabyte:3.8.0-yb-x Aug 5, 2021
Copy link
Copy Markdown

@nchandrappa nchandrappa left a comment

Choose a reason for hiding this comment

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

LGTM.

@ebyhr ebyhr deleted the delegating-cluster-npe branch August 5, 2021 11:00
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.

3 participants