[Improvement] Possible null pointer exception in CatalogRegister.java

[justinmclean]

What would you like to be improved?

In checkCatalogExist, failedException is set to null and then thrown which could cause a NPE if it is not set in the exception block.

How should we improve?

Move throw failedException; to the correct place in the catch block?

[simarjeetss]

Hello @justinmclean , can I have a look on this? I think I can resolve this.

[justinmclean]

Thanks for taking a look I have assigned it to you

[simarjeetss]

I am thinking of removing the failedException variable, moving exception handling inside the retry loop, and directly throwing a TrinoException when retries are exhausted. I also thought of adding proper InterruptedException handling. I think these modifications can enhance the method's robustness and error management.

What do you think about this?

[simarjeetss]

I made some changes to fix the NPE and improve the method. Please have a look :-
#4201

[yuqi1129]

@simarjeetss
I suggest you check similar problem in the whole project and fix them together.

[justinmclean]

This as far as I know is the only case where a NPE could happen in this way

[simarjeetss]

can u elaborate on this please

[justinmclean]

@yuqi1129 are you aware of any other cases where this might occur?

[yuqi1129]

@justinmclean
I haven't found any similar issues yet. Perhaps I need to use a code quality tool to check the project. If you find itsatisfactory now, please proceed.

[diqiu50]

@simarjeetss I think we can initialize failedException to avoid this problem.
···
Exception failedException = new TrinoException(GENERIC_INTERNAL_ERROR)
···

[justinmclean]

@simarjeetss what do you think?

[justinmclean]

@simarjeetss Do you want to still work on this?

[simarjeetss]

Yes, I'll continue working on this right now. I've been busy with other important work lately. Sorry for the inconvenience