Closed shvachko closed 9 years ago
I have given this issue to Wei-Lin. He is not in the system yet so I am temporarily assigning it to me to avoid confusion.
The patch looks good. Few minor things:
import static GiraffaConfiguration.getGiraffaTableName
. Then you can call getGiraffaTableName()
instaed of GiraffaConfiguration.getGiraffaTableName()
.Revision done at 3rd commit. Thanks.
I was not precise about (2), which meant not to introduce new long lines, while the long lines unrelated to the patch should remain as is. We can file a different issue to reformat all long lines. But this one should stick to its goal, that is introduce getGiraffaTableName()
. So let's go over one more iteration here. Minimizing code changes will be important, when we will have to support multiple versions, and changes should be promoted between them.
Thanks Konstantin. The revised code is ready at 4th commit
Found two more places where we can use getGiraffaTableName()
: htable.jsp
and index.jsp
. See under resources/hbase-webapps/giraffa
Updated at 5th commit. Thanks.
Now you can remove import GiraffaConfiguration
from the two jsp files.
Looks good otherwise.
Also Wei-Lin you need to install the pre-commit hook. We use git commits to generate release notes, so the format of log messages is important.
I removed the imports of GiraffaConfiguration, and committed. Congratulations Wei-Lin.
Giraffa table name is used in many places. Let's unify this by introducing: