[GitHub] accumulo pull request #200: Accumulo 4558

classic Classic list List threaded Threaded
24 messages Options
12
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #200: Accumulo 4558

ctubbsii
GitHub user lstav opened a pull request:

    https://github.com/apache/accumulo/pull/200

    Accumulo 4558

    Fixes ACCUMULO-4558, the shell now has a command to display tablet server status.
   
    After I started this pull request, I realized that some of the changes come from #198, so I think that merging this should also close #198.

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/lstav/accumulo ACCUMULO-4558

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/accumulo/pull/200.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #200
   
----
commit ecd4234aae90a5b1971f06c36a454274d17f15d8
Author: Luis Tavarez <[hidden email]>
Date:   2016-12-30T14:11:56Z

    ACCUMULO-4061 Tablet Servers now save the version number when they get created

commit ff8e3b86962d12629a5ae5cfd775a2c044f65fb6
Author: Luis Tavarez <[hidden email]>
Date:   2017-01-04T15:26:05Z

    ACCUMULO-4061 Modified thrift file

commit c763e5751da53331abd30c795ff454ce733ef28b
Author: Luis Tavarez <[hidden email]>
Date:   2017-01-04T15:26:38Z

    ACCUMULO-4061 Re-generated  java file

commit 67c9433e13605d6521266d1a6de0f3ebf6ec15f4
Author: Luis Tavarez <[hidden email]>
Date:   2017-01-04T15:32:33Z

    ACCUMULO-4061 Removed set method

commit c92b51e27ab483319c9b01ac0895f3cf7e57a3f4
Author: Luis Tavarez <[hidden email]>
Date:   2017-01-04T20:03:34Z

    ACCUMULO-4061 Ping on the shell returns Accumulo version

commit 1383a66ea3475bd40cb57574955fd3705a533062
Author: Luis Tavarez <[hidden email]>
Date:   2017-01-05T13:35:31Z

    ACCUMULO-4061 Mock version method now returns Constants.VERSION

commit 9b6bbf3ece40e852cb442cb7f1cac6c21c85b950
Author: Luis Tavarez <[hidden email]>
Date:   2017-01-06T13:29:25Z

    ACCUMULO-4558 Added new command to display tablet server status

commit 96a78643f7773cbe6585ed017b06e238204cb91d
Author: Luis Tavarez <[hidden email]>
Date:   2017-01-09T13:40:10Z

    ACCUMULO-4558 Modified the output so that it is similar to the config command output

commit 2061b33ace42743c159f6cc99fc63df9f8478ff5
Author: Luis Tavarez <[hidden email]>
Date:   2017-01-09T17:10:23Z

    ACCUMULO-4558 Moved TableInfoUtil to util folder, fixed the imports to point to it

commit be9154ec8a7ccd3dc9aa0d88cd03fd740f0715e5
Author: Luis Tavarez <[hidden email]>
Date:   2017-01-11T18:42:15Z

    ACCUMULO-4558 Added tabletServerNotFoundException when looking for a server

----


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #200: Accumulo 4558 Added shell command to display ser...

ctubbsii
Github user keith-turner commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/200#discussion_r95664036
 
    --- Diff: core/src/main/java/org/apache/accumulo/core/client/TServerStatus.java ---
    @@ -0,0 +1,182 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one or more
    + * contributor license agreements.  See the NOTICE file distributed with
    + * this work for additional information regarding copyright ownership.
    + * The ASF licenses this file to You under the Apache License, Version 2.0
    + * (the "License"); you may not use this file except in compliance with
    + * the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +package org.apache.accumulo.core.client;
    +
    +/**
    + * This exception is thrown if a table is deleted after an operation starts.
    + *
    + * For example if table A exist when a scan is started, but is deleted during the scan then this exception is thrown.
    + *
    + */
    +
    +public class TServerStatus extends RuntimeException {
    --- End diff --
   
    Why does this extend runtime exception?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #200: Accumulo 4558 Added shell command to display ser...

ctubbsii
In reply to this post by ctubbsii
Github user keith-turner commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/200#discussion_r95664281
 
    --- Diff: core/src/main/java/org/apache/accumulo/core/client/TServerStatus.java ---
    @@ -0,0 +1,182 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one or more
    + * contributor license agreements.  See the NOTICE file distributed with
    + * this work for additional information regarding copyright ownership.
    + * The ASF licenses this file to You under the Apache License, Version 2.0
    + * (the "License"); you may not use this file except in compliance with
    + * the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +package org.apache.accumulo.core.client;
    +
    +/**
    + * This exception is thrown if a table is deleted after an operation starts.
    + *
    + * For example if table A exist when a scan is started, but is deleted during the scan then this exception is thrown.
    + *
    + */
    +
    +public class TServerStatus extends RuntimeException {
    +
    +  private static final long serialVersionUID = 1L;
    +  private String name, version;
    +  int hostedTablets;
    +  long lastContact, entries, holdTime;
    +  double ingest, query, indexHitRate, dataHitRate, osLoad;
    +  Integer scans, minor, major;
    +
    +  public TServerStatus() {
    +    name = "";
    +    hostedTablets = 0;
    +    lastContact = 0l;
    +    entries = 0l;
    +    ingest = 0.0;
    +    query = 0.0;
    +    holdTime = 0l;
    +    scans = null;
    +    minor = null;
    +    major = null;
    +    indexHitRate = 0.0;
    +    dataHitRate = 0.0;
    +    osLoad = 0.0;
    +    version = "";
    +  }
    +
    +  public TServerStatus(String name, int hostedTablets, long lastContact, long entries, double ingest, double query, long holdTime, Integer scans,
    +      Integer minor, Integer major, double indexHitRate, double dataHitRate, double osLoad, String version) {
    +    this.name = name;
    +    this.hostedTablets = hostedTablets;
    +    this.lastContact = lastContact;
    +    this.entries = entries;
    +    this.ingest = ingest;
    +    this.query = query;
    +    this.holdTime = holdTime;
    +    this.scans = scans;
    +    this.minor = minor;
    +    this.major = major;
    +    this.indexHitRate = indexHitRate;
    +    this.dataHitRate = dataHitRate;
    +    this.osLoad = osLoad;
    +    this.version = version;
    +  }
    +
    +  public String getName() {
    +    return name;
    +  }
    +
    +  public void setName(String name) {
    +    this.name = name;
    +  }
    +
    +  public String getVersion() {
    +    return version;
    +  }
    +
    +  public void setVersion(String version) {
    +    this.version = version;
    +  }
    +
    +  public int getHostedTablets() {
    +    return hostedTablets;
    +  }
    +
    +  public void setHostedTablets(int hostedTablets) {
    +    this.hostedTablets = hostedTablets;
    +  }
    +
    +  public long getLastContact() {
    +    return lastContact;
    +  }
    +
    +  public void setLastContact(long lastContact) {
    +    this.lastContact = lastContact;
    +  }
    +
    +  public long getEntries() {
    +    return entries;
    +  }
    +
    +  public void setEntries(long entries) {
    +    this.entries = entries;
    +  }
    +
    +  public long getHoldTime() {
    +    return holdTime;
    +  }
    +
    +  public void setHoldTime(long holdTime) {
    +    this.holdTime = holdTime;
    +  }
    +
    +  public double getIngest() {
    +    return ingest;
    +  }
    +
    +  public void setIngest(double ingest) {
    +    this.ingest = ingest;
    +  }
    +
    +  public double getQuery() {
    +    return query;
    +  }
    +
    +  public void setQuery(double query) {
    +    this.query = query;
    +  }
    +
    +  public double getIndexHitRate() {
    --- End diff --
   
    Metrics like this are tightly coupled to our current implementation in the tserver.  If we want to change how caching works in the tserver in the future, having this in the public API could be problematic.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #200: Accumulo 4558 Added shell command to display ser...

ctubbsii
In reply to this post by ctubbsii
Github user keith-turner commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/200#discussion_r95664526
 
    --- Diff: core/src/main/java/org/apache/accumulo/core/client/admin/InstanceOperations.java ---
    @@ -81,6 +82,14 @@
       List<String> getTabletServers();
     
       /**
    +   * List the tablet server status
    +   *
    +   * @return A list of tablet server status.
    +   */
    +
    +  List<TServerStatus> getTabletServerStatus() throws AccumuloException;
    --- End diff --
   
    All API additions should have `@since 2.0.0` javadoc


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #200: Accumulo 4558 Added shell command to display ser...

ctubbsii
In reply to this post by ctubbsii
Github user lstav commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/200#discussion_r95779971
 
    --- Diff: core/src/main/java/org/apache/accumulo/core/client/TServerStatus.java ---
    @@ -0,0 +1,182 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one or more
    + * contributor license agreements.  See the NOTICE file distributed with
    + * this work for additional information regarding copyright ownership.
    + * The ASF licenses this file to You under the Apache License, Version 2.0
    + * (the "License"); you may not use this file except in compliance with
    + * the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +package org.apache.accumulo.core.client;
    +
    +/**
    + * This exception is thrown if a table is deleted after an operation starts.
    + *
    + * For example if table A exist when a scan is started, but is deleted during the scan then this exception is thrown.
    + *
    + */
    +
    +public class TServerStatus extends RuntimeException {
    --- End diff --
   
    I was following what I saw on the code in the other classes of the o.a.a.core.client package. Will remove it, thanks for pointing it out.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #200: Accumulo 4558 Added shell command to display ser...

ctubbsii
In reply to this post by ctubbsii
Github user lstav commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/200#discussion_r95780078
 
    --- Diff: core/src/main/java/org/apache/accumulo/core/client/admin/InstanceOperations.java ---
    @@ -81,6 +82,14 @@
       List<String> getTabletServers();
     
       /**
    +   * List the tablet server status
    +   *
    +   * @return A list of tablet server status.
    +   */
    +
    +  List<TServerStatus> getTabletServerStatus() throws AccumuloException;
    --- End diff --
   
    Thanks, didn't know about that.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #200: Accumulo 4558 Added shell command to display ser...

ctubbsii
In reply to this post by ctubbsii
Github user lstav commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/200#discussion_r95780972
 
    --- Diff: core/src/main/java/org/apache/accumulo/core/client/TServerStatus.java ---
    @@ -0,0 +1,182 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one or more
    + * contributor license agreements.  See the NOTICE file distributed with
    + * this work for additional information regarding copyright ownership.
    + * The ASF licenses this file to You under the Apache License, Version 2.0
    + * (the "License"); you may not use this file except in compliance with
    + * the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +package org.apache.accumulo.core.client;
    +
    +/**
    + * This exception is thrown if a table is deleted after an operation starts.
    + *
    + * For example if table A exist when a scan is started, but is deleted during the scan then this exception is thrown.
    + *
    + */
    +
    +public class TServerStatus extends RuntimeException {
    +
    +  private static final long serialVersionUID = 1L;
    +  private String name, version;
    +  int hostedTablets;
    +  long lastContact, entries, holdTime;
    +  double ingest, query, indexHitRate, dataHitRate, osLoad;
    +  Integer scans, minor, major;
    +
    +  public TServerStatus() {
    +    name = "";
    +    hostedTablets = 0;
    +    lastContact = 0l;
    +    entries = 0l;
    +    ingest = 0.0;
    +    query = 0.0;
    +    holdTime = 0l;
    +    scans = null;
    +    minor = null;
    +    major = null;
    +    indexHitRate = 0.0;
    +    dataHitRate = 0.0;
    +    osLoad = 0.0;
    +    version = "";
    +  }
    +
    +  public TServerStatus(String name, int hostedTablets, long lastContact, long entries, double ingest, double query, long holdTime, Integer scans,
    +      Integer minor, Integer major, double indexHitRate, double dataHitRate, double osLoad, String version) {
    +    this.name = name;
    +    this.hostedTablets = hostedTablets;
    +    this.lastContact = lastContact;
    +    this.entries = entries;
    +    this.ingest = ingest;
    +    this.query = query;
    +    this.holdTime = holdTime;
    +    this.scans = scans;
    +    this.minor = minor;
    +    this.major = major;
    +    this.indexHitRate = indexHitRate;
    +    this.dataHitRate = dataHitRate;
    +    this.osLoad = osLoad;
    +    this.version = version;
    +  }
    +
    +  public String getName() {
    +    return name;
    +  }
    +
    +  public void setName(String name) {
    +    this.name = name;
    +  }
    +
    +  public String getVersion() {
    +    return version;
    +  }
    +
    +  public void setVersion(String version) {
    +    this.version = version;
    +  }
    +
    +  public int getHostedTablets() {
    +    return hostedTablets;
    +  }
    +
    +  public void setHostedTablets(int hostedTablets) {
    +    this.hostedTablets = hostedTablets;
    +  }
    +
    +  public long getLastContact() {
    +    return lastContact;
    +  }
    +
    +  public void setLastContact(long lastContact) {
    +    this.lastContact = lastContact;
    +  }
    +
    +  public long getEntries() {
    +    return entries;
    +  }
    +
    +  public void setEntries(long entries) {
    +    this.entries = entries;
    +  }
    +
    +  public long getHoldTime() {
    +    return holdTime;
    +  }
    +
    +  public void setHoldTime(long holdTime) {
    +    this.holdTime = holdTime;
    +  }
    +
    +  public double getIngest() {
    +    return ingest;
    +  }
    +
    +  public void setIngest(double ingest) {
    +    this.ingest = ingest;
    +  }
    +
    +  public double getQuery() {
    +    return query;
    +  }
    +
    +  public void setQuery(double query) {
    +    this.query = query;
    +  }
    +
    +  public double getIndexHitRate() {
    --- End diff --
   
    After talking with @ctubbsii, he mentioned that since I'm returning the tablet server status on the new API adition, I needed to create this class for that and then populate it with the actual TabletServerStatus object, which I do on `IntanceOperationsImpl.java`. This class only stores the values for the public API. If I misunderstood what you meant, please let me know.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #200: Accumulo 4558 Added shell command to display ser...

ctubbsii
In reply to this post by ctubbsii
Github user ctubbsii commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/200#discussion_r95804314
 
    --- Diff: shell/src/main/java/org/apache/accumulo/shell/commands/TabletServerStatusCommand.java ---
    @@ -0,0 +1,95 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one or more
    + * contributor license agreements.  See the NOTICE file distributed with
    + * this work for additional information regarding copyright ownership.
    + * The ASF licenses this file to You under the Apache License, Version 2.0
    + * (the "License"); you may not use this file except in compliance with
    + * the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +package org.apache.accumulo.shell.commands;
    +
    +import java.util.ArrayList;
    +import java.util.List;
    +
    +import org.apache.accumulo.core.client.TServerStatus;
    +import org.apache.accumulo.core.client.TabletServerNotFoundException;
    +import org.apache.accumulo.core.client.admin.InstanceOperations;
    +import org.apache.accumulo.shell.Shell;
    +import org.apache.accumulo.shell.Shell.Command;
    +import org.apache.commons.cli.CommandLine;
    +import org.apache.commons.cli.MissingOptionException;
    +import org.apache.commons.cli.Option;
    +import org.apache.commons.cli.Options;
    +
    +/**
    + *
    + */
    +public class TabletServerStatusCommand extends Command {
    +
    +  private Option tserverOption, disablePaginationOpt, allOption;
    +
    +  @Override
    +  public String description() {
    +    return "get tablet servers status, master server must be running";
    +  }
    +
    +  @Override
    +  public int execute(final String fullCommand, final CommandLine cl, final Shell shellState) throws Exception {
    +    List<TServerStatus> tservers;
    +
    +    final InstanceOperations instanceOps = shellState.getConnector().instanceOperations();
    +
    +    final boolean paginate = !cl.hasOption(disablePaginationOpt.getOpt());
    +
    +    if (cl.hasOption(tserverOption.getOpt())) {
    +      tservers = new ArrayList<>();
    +      for (TServerStatus ts : instanceOps.getTabletServerStatus()) {
    +        if (ts.getName().equals(cl.getOptionValue(tserverOption.getOpt()))) {
    +          tservers.add(ts);
    +        }
    +      }
    +    } else if (cl.hasOption(allOption.getOpt())) {
    +      tservers = instanceOps.getTabletServerStatus();
    +    } else {
    +      throw new MissingOptionException("Missing options");
    +    }
    +
    +    if (tservers.isEmpty()) {
    +      throw new TabletServerNotFoundException("Tablet Servers not found");
    --- End diff --
   
    I don't think we need to throw an exception here. We can just print nothing, if there's no status available.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo issue #200: Accumulo 4558 Added shell command to display server sta...

ctubbsii
In reply to this post by ctubbsii
Github user mjwall commented on the issue:

    https://github.com/apache/accumulo/pull/200
 
    @keith-turner @ctubbsii is this ready to go in your opinion?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #200: Accumulo 4558 Added shell command to display ser...

ctubbsii
In reply to this post by ctubbsii
Github user keith-turner commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/200#discussion_r106443306
 
    --- Diff: core/src/main/java/org/apache/accumulo/core/client/admin/InstanceOperations.java ---
    @@ -81,6 +81,15 @@
       List<String> getTabletServers();
     
       /**
    +   * List the tablet server status
    +   *
    +   * @return A list of tablet server status.
    +   * @since 2.0.0
    +   */
    +
    +  List<Map<String,String>> getTabletServerStatus() throws AccumuloException;
    --- End diff --
   
    From an API design standpoint I would rather see a class returned as that gives more flexibility to evolves the API in the future.  This is a lesson I have learned the hard way.   For example if I wanted to add functionality to get the description of a stat or historical information in the future, that would mean adding more methods to intstance operations.
   
    I would rather see something like the following for the API.
   
    ```java
    static interfce TabletServerID {
      public String getHost();
      public int getPort();
      public long getSessionId();
    }
   
    static interface TabletServersStatus {
      List<TabletServerID> getTabletServers();
      Map<String,String> getTabletServerStatus(TabletServerID tsid);
    }
   
    TabletServersStatus getTabletServerStatus();
    ```
   



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #200: Accumulo 4558 Added shell command to display ser...

ctubbsii
In reply to this post by ctubbsii
Github user keith-turner commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/200#discussion_r106501794
 
    --- Diff: core/src/main/java/org/apache/accumulo/core/client/admin/InstanceOperations.java ---
    @@ -81,6 +81,15 @@
       List<String> getTabletServers();
     
       /**
    +   * List the tablet server status
    +   *
    +   * @return A list of tablet server status.
    +   * @since 2.0.0
    +   */
    +
    +  List<Map<String,String>> getTabletServerStatus() throws AccumuloException;
    --- End diff --
   
    Introducing a TabletServerID would be inconsistent with the rest of the API which currently uses a string to identify tablet servers.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo issue #200: Accumulo 4558 Added shell command to display server sta...

ctubbsii
In reply to this post by ctubbsii
Github user ctubbsii commented on the issue:

    https://github.com/apache/accumulo/pull/200
 
    @mjwall I'll defer to @keith-turner regarding the design of the new public API method. I can take another look after that.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo issue #200: Accumulo 4558 Added shell command to display server sta...

ctubbsii
In reply to this post by ctubbsii
Github user lstav commented on the issue:

    https://github.com/apache/accumulo/pull/200
 
    I just pushed the rebasing onto master and recompiling of thrift.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo issue #200: Accumulo 4558 Added shell command to display server sta...

ctubbsii
In reply to this post by ctubbsii
Github user lstav commented on the issue:

    https://github.com/apache/accumulo/pull/200
 
    I just fixed the conflict with the main branch (it was due to the change being done to the old monitor code), I spoke with @milleruntime offline and he might take over fixing what had been discussed on this PR.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo issue #200: Accumulo 4558 Added shell command to display server sta...

ctubbsii
In reply to this post by ctubbsii
Github user milleruntime commented on the issue:

    https://github.com/apache/accumulo/pull/200
 
    Discussed this some today offline with the team and there seems to be resistance to creating new API methods for this change.  @lstav you could pull out the changes for getting the version for ACCUMULO-4061


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo issue #200: Accumulo 4558 Added shell command to display server sta...

ctubbsii
In reply to this post by ctubbsii
Github user lstav commented on the issue:

    https://github.com/apache/accumulo/pull/200
 
    @milleruntime you mean to just leave the status report on the shell? if so I can do that.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo issue #200: Accumulo 4558 Added shell command to display server sta...

ctubbsii
In reply to this post by ctubbsii
Github user ctubbsii commented on the issue:

    https://github.com/apache/accumulo/pull/200
 
    What @milleruntime was referring to was a conversation with @keith-turner and myself (a small subset of the overall Accumulo "team", but a team in another sense). In that conversation, I had suggested we omit the shell and API changes *for now*, and limit the change to updating the Thrift to include the version, so that the monitor can display it in a REST endpoint. That way, there's still a way to access the version information and other tablet server status details programatically, but no need to decide on a particular public API just yet, and no need to add any new command to the shell either, until the public API exists.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo issue #200: Accumulo 4558 Added shell command to display server sta...

ctubbsii
In reply to this post by ctubbsii
Github user lstav commented on the issue:

    https://github.com/apache/accumulo/pull/200
 
    Sounds good, I'll probably create a new PR to add the Thrift update to include the version, that way I can keep the Shell changes as a reference for when we decide to add the new command.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo issue #200: Accumulo 4558 Added shell command to display server sta...

ctubbsii
In reply to this post by ctubbsii
Github user ctubbsii commented on the issue:

    https://github.com/apache/accumulo/pull/200
 
    @lstav I was thinking we could just save the commit as an attachment to the JIRA, for future reference. That way, we're not relying on an old PR if we revisit it, and can move on with another PR or just pull out what we want out of this one.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo issue #200: Accumulo 4558 Added shell command to display server sta...

ctubbsii
In reply to this post by ctubbsii
Github user lstav commented on the issue:

    https://github.com/apache/accumulo/pull/200
 
    @ctubbsii that sounds like a good idea to me, would you mind adding the commit to JIRA?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
12
Loading...